Bug 127605

Summary: Web Inspector: Move InspectorRuntimeAgent into JavaScriptCore
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, commit-queue, ggaren, graouts, gyuyoung.kim, joepeck, mark.lam, rakuco, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
timothy: review+
[PATCH] For Bots 1 none

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>.