Bug 197439

Summary: RemoteInspector::updateAutomaticInspectionCandidate should have a default implementation.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: PlatformAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: Basuke.Suzuki, commit-queue, don.olmstead, ews-watchlist, hi, joepeck, keith_miller, mark.lam, mcatanzaro, msaboff, ross.kirsling, saam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 197029    
Attachments:
Description Flags
PATCH
none
PATCH
none
PATCH
none
PATCH
none
Patch
none
Patch
none
Patch for landing none

Description Basuke Suzuki 2019-04-30 14:01:00 PDT
The required method RemoteInspector::updateAutomaticInspectionCandidate() was not implemented.
Comment 1 Basuke Suzuki 2019-04-30 14:06:25 PDT
Created attachment 368608 [details]
PATCH
Comment 2 Basuke Suzuki 2019-04-30 14:08:19 PDT
Created attachment 368611 [details]
PATCH
Comment 3 Ross Kirsling 2019-04-30 15:47:26 PDT
Looks like this is a copy-paste of the Glib implementation -- unless we have an expectation for the two to diverge, perhaps it should be moved to a common location instead?
Comment 4 Basuke Suzuki 2019-04-30 16:17:44 PDT
(In reply to Ross Kirsling from comment #3)
> Looks like this is a copy-paste of the Glib implementation -- unless we have
> an expectation for the two to diverge, perhaps it should be moved to a
> common location instead?

Humm, good point. I didn't think about that before because it was separated before we've start adding our port. It seems almost neutral for all port. I'll try moving it in shared implementation.
Comment 5 Basuke Suzuki 2019-04-30 16:39:13 PDT
Created attachment 368628 [details]
PATCH
Comment 6 Basuke Suzuki 2019-04-30 17:41:15 PDT
Created attachment 368635 [details]
PATCH
Comment 7 Basuke Suzuki 2019-04-30 17:42:27 PDT
Comment on attachment 368635 [details]
PATCH

At this moment, this is a good balance to share code. We'll do more when we implement automatic inspection.
Comment 8 Don Olmstead 2019-04-30 17:48:41 PDT
(In reply to Basuke Suzuki from comment #7)
> Comment on attachment 368635 [details]
> PATCH
> 
> At this moment, this is a good balance to share code. We'll do more when we
> implement automatic inspection.

I'd probably change the bug name at this time since this is touching more than playstation.
Comment 9 Don Olmstead 2019-05-01 12:26:36 PDT
Comment on attachment 368635 [details]
PATCH

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:107
> +    if (updateTargetInternal())

This should have `target` passed in then it should build.
Comment 10 Ross Kirsling 2019-05-01 13:52:32 PDT
Created attachment 368703 [details]
Patch
Comment 11 Ross Kirsling 2019-05-01 15:26:59 PDT
Created attachment 368721 [details]
Patch
Comment 12 Devin Rousso 2019-05-01 15:47:35 PDT
Comment on attachment 368721 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:113
> +bool RemoteInspector::updateTargetInternal(RemoteControllableTarget* target)

This is an odd name.  How about `updateListingForTarget` or `updateTargetMap`?

> Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:-111
> -    {

NIT: I realize this block is unnecessary, but I wouldn't change it for the sake of keeping the diff cleaner :P
Comment 13 Ross Kirsling 2019-05-01 16:25:43 PDT
(In reply to Devin Rousso from comment #12)
> Comment on attachment 368721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368721&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:113
> > +bool RemoteInspector::updateTargetInternal(RemoteControllableTarget* target)
> 
> This is an odd name.  How about `updateListingForTarget` or
> `updateTargetMap`?

I almost changed this to `updateTargetListings` but it turns out that `updateTargetListing` is already a method! That one literally just updates m_targetListingMap, whereas this one updates m_targetMap then sets OR clears m_targetListingMap. I suppose `updateTargetMap` could be clearer if that were taken to imply an update to the listing too -- do you think that's the case?

> > Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:-111
> > -    {
> 
> NIT: I realize this block is unnecessary, but I wouldn't change it for the
> sake of keeping the diff cleaner :P

I would have left it, but since the Glib version doesn't have it, I thought it'd be okay to remove it while merging implementations, haha.
Comment 14 Ross Kirsling 2019-05-01 17:03:05 PDT
Created attachment 368737 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2019-05-01 17:25:52 PDT
Comment on attachment 368737 [details]
Patch for landing

Clearing flags on attachment: 368737

Committed r244861: <https://trac.webkit.org/changeset/244861>
Comment 16 WebKit Commit Bot 2019-05-01 17:25:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-05-02 15:35:33 PDT
<rdar://problem/50424864>