Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 481913002: [DevTools] Merge InspectorFrontendClientImpl into WebDevToolsFrontendImpl and remove unused code. (Closed)

Created:
6 years, 4 months ago by dgozman
Modified:
6 years, 4 months ago
CC:
blink-reviews, jamesr, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org
Project:
blink
Visibility:
Public.

Description

[DevTools] Merge InspectorFrontendClientImpl into WebDevToolsFrontendImpl and remove unused code. BUG=none Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180464

Patch Set 1 #

Patch Set 2 : rebase #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -375 lines) Patch
M Source/core/inspector/InspectorController.h View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/inspector/InspectorController.cpp View 3 chunks +2 lines, -2 lines 0 comments Download
M Source/core/inspector/InspectorFrontendClient.h View 1 chunk +1 line, -4 lines 0 comments Download
D Source/web/InspectorFrontendClientImpl.h View 1 chunk +0 lines, -70 lines 0 comments Download
D Source/web/InspectorFrontendClientImpl.cpp View 1 chunk +0 lines, -178 lines 0 comments Download
M Source/web/WebDevToolsFrontendImpl.h View 1 chunk +16 lines, -21 lines 3 comments Download
M Source/web/WebDevToolsFrontendImpl.cpp View 1 1 chunk +109 lines, -91 lines 0 comments Download
M Source/web/web.gypi View 1 chunk +0 lines, -2 lines 0 comments Download
M public/web/WebDevToolsFrontend.h View 1 chunk +4 lines, -5 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
dgozman
Could you please take a look?
6 years, 4 months ago (2014-08-18 13:03:21 UTC) #1
sof
Oilpan usage (what there is of it now) looks fine. https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h File Source/web/WebDevToolsFrontendImpl.h (right): https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h#newcode65 ...
6 years, 4 months ago (2014-08-18 14:13:49 UTC) #2
pfeldman
lgtm
6 years, 4 months ago (2014-08-18 14:26:24 UTC) #3
dgozman
https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h File Source/web/WebDevToolsFrontendImpl.h (right): https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h#newcode65 Source/web/WebDevToolsFrontendImpl.h:65: RefPtrWillBePersistent<InspectorFrontendHost> m_frontendHost; On 2014/08/18 14:13:49, sof wrote: > I ...
6 years, 4 months ago (2014-08-18 15:03:40 UTC) #4
dgozman
The CQ bit was checked by dgozman@chromium.org
6 years, 4 months ago (2014-08-18 15:03:51 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/481913002/20001
6 years, 4 months ago (2014-08-18 15:04:29 UTC) #6
haraken
https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h File Source/web/WebDevToolsFrontendImpl.h (right): https://codereview.chromium.org/481913002/diff/20001/Source/web/WebDevToolsFrontendImpl.h#newcode65 Source/web/WebDevToolsFrontendImpl.h:65: RefPtrWillBePersistent<InspectorFrontendHost> m_frontendHost; On 2014/08/18 15:03:40, dgozman wrote: > On ...
6 years, 4 months ago (2014-08-18 15:06:04 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (20001) as 180464
6 years, 4 months ago (2014-08-18 15:08:23 UTC) #8
dgozman
6 years, 4 months ago (2014-08-18 15:35:22 UTC) #9
Message was sent while issue was closed. 
> Can we use WebPrivatePtr<InspectorFrontendHost> here?
> 
> See WebMediaStream.h and other web/XXX.h that uses WebPrivatePtr<XXX>.

It seems that WebPrivatePtr is for objects that are just lightweight wrappers
around core/ objects.
WebDevToolsFrontend is heavy implementation object, and it also has a client and
calls methods on the client. I haven't find any examples of mixing WebPrivatePtr
together with client in public/web. For example, WebSocket/WebSocketImpl does
the same as this patch.
I'm not sure what's the right way. Perhaps, all the interfaces should follow a
single pattern?

Powered by Google App Engine
This is Rietveld 408576698