Bug 62118

Summary: Use of ENABLE(INSPECTOR) is inconsistent
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mnaganov, pfeldman, steveblock, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Steve Block
Reported 2011-06-06 03:47:48 PDT
Android does not enable INSPECTOR and this results in build errors after http://trac.webkit.org/changeset/81265 http://trac.webkit.org/changeset/81265 added a call to ScriptProfiler::initialize() from V8DOMWindowShell::initContextIfNeeded() (see also Bug 62014). ScriptProfiler requires ScriptProfile, which in turn depends upon InspectorObject. InspectorObject is provided in InspectorValues.h, but the contents are guarded with ENABLE(INSPECTOR), which Android does not enable. We could simply guard the call to ScriptProfiler::initialize() from V8DOMWindowShell::initContextIfNeeded() with ENABLE(INSPECTOR), or fix what seems to be a more general problem with the use of ENABLE(INSPECTOR).
Attachments
Patch (1.24 KB, patch)
2011-06-06 03:53 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (1.45 KB, patch)
2011-06-06 05:47 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (1.46 KB, patch)
2011-06-06 05:48 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Steve Block
Comment 1 2011-06-06 03:53:19 PDT
Created attachment 96074 [details] Patch
Mikhail Naganov
Comment 2 2011-06-06 04:07:32 PDT
Yes, I think, it's acceptable. Yury, can you please review this?
Yury Semikhatsky
Comment 3 2011-06-06 04:34:46 PDT
Comment on attachment 96074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96074&action=review > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:301 > +#if ENABLE(INSPECTOR) If ScriptProfiler call is guarded with ENABLE(INSPECTOR) here then corresponding part of ScriptProfiler implementation should also be guarded, r- for this. Also, I'd rather keep all guards in ScriptProfiler.cpp
Steve Block
Comment 4 2011-06-06 04:57:12 PDT
> Also, I'd rather keep all guards in ScriptProfiler.cpp So we should guard the contents of ScriptProfiler::initialize()? The problem is that I can't compile ScriptProfiler without also compiling ScriptProfile, but this requires InspectorObject, which is guarded by ENABLE(INSPECTOR). Should all of other methods of ScriptProfiler be guarded by EANBLE(INSPECTOR)?
Yury Semikhatsky
Comment 5 2011-06-06 05:07:56 PDT
(In reply to comment #4) > > Also, I'd rather keep all guards in ScriptProfiler.cpp > So we should guard the contents of ScriptProfiler::initialize()? > I'd do so, but I don't mind if you leave the guard at the call site as well. We regularly place guards at call sites and around whole implementation of the class but I think it won't work in this case as part of ScriptProfiler should be available when inspector is disabled. > The problem is that I can't compile ScriptProfiler without also compiling ScriptProfile, but this requires InspectorObject, which is guarded by ENABLE(INSPECTOR). Should all of other methods of ScriptProfiler be guarded by EANBLE(INSPECTOR)? Yes, all methods that are only used by inspector should be guarded by ENABLE(INSPECTOR), this should make ScriptProfiler compile even when inspector is disabled.
Steve Block
Comment 6 2011-06-06 05:47:18 PDT
Created attachment 96079 [details] Patch
Steve Block
Comment 7 2011-06-06 05:48:31 PDT
Created attachment 96080 [details] Patch
WebKit Commit Bot
Comment 8 2011-06-06 07:14:05 PDT
The commit-queue encountered the following flaky tests while processing attachment 96080 [details]: inspector/debugger/source-frame.html bug 57399 (author: podivilov@chromium.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 9 2011-06-06 07:15:45 PDT
Comment on attachment 96080 [details] Patch Clearing flags on attachment: 96080 Committed r88166: <http://trac.webkit.org/changeset/88166>
WebKit Commit Bot
Comment 10 2011-06-06 07:15:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.