Bug 197439 – RemoteInspector::updateAutomaticInspectionCandidate should have a default implementation.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

The required method RemoteInspector::updateAutomaticInspectionCandidate() was not implemented.

[Basuke Suzuki]

Created attachment 368608 [details]
PATCH

[Basuke Suzuki]

Created attachment 368611 [details]
PATCH

[Ross Kirsling]

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?

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 368628 [details]
PATCH

[Basuke Suzuki]

Created attachment 368635 [details]
PATCH

[Basuke Suzuki]

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.

[Don Olmstead]

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

[Don Olmstead]

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.

[Ross Kirsling]

Created attachment 368703 [details]
Patch

[Ross Kirsling]

Created attachment 368721 [details]
Patch

[Devin Rousso]

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

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 368737 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 368737 [details]
Patch for landing

Clearing flags on attachment: 368737

Committed r244861: <https://trac.webkit.org/changeset/244861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50424864>