Bug 127605 - Web Inspector: Move InspectorRuntimeAgent into JavaScriptCore
Summary: Web Inspector: Move InspectorRuntimeAgent into JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-24 20:25 PST by Joseph Pecoraro
Modified: 2014-01-25 09:44 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (60.21 KB, patch)
2014-01-24 20:31 PST, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[PATCH] For Bots 1 (60.05 KB, patch)
2014-01-24 20:39 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-01-24 20:25:16 PST
All requirements (InjectedScripts and ScriptDebugServer) are met. We can now move down InspectorRuntimeAgent.
Comment 1 Radar WebKit Bug Importer 2014-01-24 20:25:24 PST
<rdar://problem/15908355>
Comment 2 Joseph Pecoraro 2014-01-24 20:31:27 PST
Created attachment 222189 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2014-01-24 20:39:56 PST
Created attachment 222190 [details]
[PATCH] For Bots 1
Comment 4 WebKit Commit Bot 2014-01-24 20:42:54 PST
Attachment 222190 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/inspector/WorkerRuntimeAgent.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Timothy Hatcher 2014-01-24 21:05:59 PST
Comment on attachment 222189 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=222189&action=review

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2013, 2014?

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2014

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.h:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/inspector/PageRuntimeAgent.cpp:56
> +PageRuntimeAgent::PageRuntimeAgent(InjectedScriptManager* injectedScriptManager, Page* page, InspectorPageAgent* pageAgent)

We might want to consider putting Inspector back in these subclass names. Page or Web?

> Source/WebCore/inspector/PageRuntimeAgent.h:70
> +    virtual JSC::VM* globalVM() override;

final too?

> Source/WebCore/inspector/WorkerRuntimeAgent.h:59
> +    virtual JSC::VM* globalVM() override;

Ditto.
Comment 6 Timothy Hatcher 2014-01-24 21:06:54 PST
Comment on attachment 222189 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=222189&action=review

> Source/WebCore/inspector/PageRuntimeAgent.h:56
> +class PageRuntimeAgent final : public Inspector::InspectorRuntimeAgent {

Oh, does final on the class apply to all the override virtual functions?
Comment 7 Joseph Pecoraro 2014-01-24 22:09:24 PST
(In reply to comment #6)
> (From update of attachment 222189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222189&action=review
> 
> > Source/WebCore/inspector/PageRuntimeAgent.h:56
> > +class PageRuntimeAgent final : public Inspector::InspectorRuntimeAgent {
> 
> Oh, does final on the class apply to all the override virtual functions?

Yep!
Comment 8 Joseph Pecoraro 2014-01-24 22:12:31 PST
> 
> > Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:2
> > + * Copyright (C) 2013 Apple Inc. All rights reserved.
> 
> 2014

Hehe, good catch on these. I had written this in 2013 though, I didn't copy/paste error it =). I'll update them.


> > Source/WebCore/inspector/PageRuntimeAgent.cpp:56
> > +PageRuntimeAgent::PageRuntimeAgent(InjectedScriptManager* injectedScriptManager, Page* page, InspectorPageAgent* pageAgent)
> 
> We might want to consider putting Inspector back in these subclass names. Page or Web?

Names could get ridiculously long. Lets keep these JSGlobalObject/Web/Page/Worker. And if we need to change we can change all at once.
Comment 9 Joseph Pecoraro 2014-01-25 09:44:20 PST
Landed <http://trac.webkit.org/changeset/162767>.