[webkit-reviews] review canceled: [Bug 202219] Web Inspector: attach to provisional pages early to not miss events during process swaps (PSON) : [Attachment 379967] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 8 10:45:46 PDT 2019
Yury Semikhatsky <yurys at chromium.org> has canceled Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 202219: Web Inspector: attach to provisional pages early to not miss events
during process swaps (PSON)
https://bugs.webkit.org/show_bug.cgi?id=202219
Attachment 379967: Patch
https://bugs.webkit.org/attachment.cgi?id=379967&action=review
--- Comment #5 from Yury Semikhatsky <yurys at chromium.org> ---
Comment on attachment 379967
--> https://bugs.webkit.org/attachment.cgi?id=379967
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review
>> Source/WebCore/ChangeLog:11
>> + (WebCore::InspectorController::setIsUnderTest): This method maybe
closed several times
>
> Typo: "maybe closed" => "may be called"
Done.
>> Source/WebInspectorUI/ChangeLog:9
>> + no changes frontend bhavior, it will wait for the provisional
target to commit
>
> Typo: "bhavior" => "behavior"
Done.
>> Source/WebInspectorUI/ChangeLog:25
>> + send commands to the provisional targtets yet, it has to ignore all
activities
>
> Typo: "targtets" => "targets"
Done.
>> Source/WebInspectorUI/ChangeLog:30
>> + Better support for provisional targets will be added to frontend in
a separate change.
>
> Can we have bug URL for this?
I was going to use the same https://webkit.org/b/202219, so that it's easier to
track what's going on across multiple patches in a single bug.
>> Source/WebInspectorUI/ChangeLog:34
>> + of tasrgetDestroyed/targetCreated events which matches previous
behavior.
>
> Typo: "tasrgetDestroyed" => "targetDestroyed"
Done.
>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:139
>> + return;
>
> Style: please add a newline after this return
Done.
>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:140
>> + InspectorTarget* target = m_targets.get(committedTargetID);
>
> NIT: `auto*`
Done.
>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:142
>> + return;
>
> Ditto (139)
Done.
>> Source/JavaScriptCore/inspector/protocol/Target.json:11
>> + { "name": "isProvisional", "type": "boolean", "optional":
true, "description": "Whether this is a provisional page target." },
>
> NIT: I prefer putting `optional` values at the end of a type's declaration,
so it's easier to read what's required vs what's optional
Done.
>> Source/JavaScriptCore/inspector/protocol/Target.json:12
>> + { "name": "previousTargetId", "type": "string", "optional":
true },
>
> Ditto
>
> Also, where is this actually used? I don't see the frontend using this value
at all. It uses `oldTargetId` from `Target.event.didCommitProvisionalTarget`
instead. Can they be merged? I'd rather have the `oldTargetId` parameter than
add another optional value to `Target.TargetInfo`.
I removed oldTargetId parameters in favor of TargetInfo.previousTargetId. I
couldn't decide first between adding it as a field to the info structure and
passing as a separate parameter in events. After talking to other folks about
the experience with DevTools procol it seems better to just put it into the
structure passed as a parameter in different target related events. It makes
receiver's code more consistent and keeps the signatures of c++ classes sane.
>> Source/WebCore/inspector/InspectorController.cpp:346
>> void InspectorController::setIsUnderTest(bool value)
>
> NIT: we could inline this in the header now that it's so small/simple
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:311
>> // Ignore errors if a worker went away quickly.
>
> NIT: please move this comment to be above the `if` so it's clear what it's
referring to
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:312
>> + WorkerAgent.sendMessageToWorker(this._workerId, message).catch(e =>
{
>
> Style: please use a full/descriptive parameter name, like `error`
> Style: we always add the `(...)` around the parameters of an arrow function,
even if there's only one argument
> ```js
> WorkerAgent.sendMessageToWorker(this._workerId, message).catch((error) =>
{
> ```
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:313
>> + if (this.target.isDestroyed())
>
> If the target is already destroyed, shouldn't we not even be sending the
command to begin with? We should probably add an assertion somewhere
`console.assert(!this.target.isDestroyed);`.
>
> Or is this to handle the situation where the target is destroyed after the
command is dispatched, but before the response is received?
It's the latter.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:315
>> + throw e;
>
> Rather than re-throw, please use `WI.reportInternalException(error)` instead.
We try to avoid using `throw` as much as possible.
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:339
>> + TargetAgent.sendMessageToTarget(this._targetId, message).catch(e =>
{
>
> Ditto
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:342
>> + if (this.target.isDestroyed())
>
> Ditto
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:344
>> + throw e;
>
> Ditto
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:208
>> + isDestroyed()
>
> Why not use a getter?
> ```js
> get isDestroyed() { return this._isDestroyed; }
> ```
Done. Normally I'd avoid getters/setters as the may hide side effects of
seemingly innocent field access. It breaks the principle of least surprise.
Basically with a method call it's obvious that it may run some logic while
member access is normally expected to be a simple cheap operation with no extra
behavior. I see that front-end uses getters/setters a lot so I'll just follow
the same style.
>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32
>> + this._swappedTargetIds = new Set;
>
> Please move this logic to `WI.TargetManager`. We try to keep our observer
classes (which are responsible for handling events received by the frontend) as
"clean" of logic as possible, and instead have the various managers and model
objects handle the actual logic.
Done. All methods of TargetOberver are pure delegates to a singleton instance
of TargetManager, what's the purpose of this class? Why not register
TargetManager as event handler directly?
>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:37
>> + if (targetInfo.isProvisional) {
>
> Please add a compatibility statement.
> ```
> // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` did not
exist yet.
Done.
>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:44
>> + didCommitProvisionalTarget(previousTargetId, targetInfo)
>
> Along the lines of 30-31, I'd create a
`WI.TargetManager.prototype.didCommitProvisionalTarget` function and move this
logic inside of that. I'd strongly prefer keeping the name the same, so it's
easier to follow the code through the various layers/transformations.
Done.
>> Source/WebInspectorUI/UserInterface/Test/Test.js:107
>> WI.transitionPageTarget = function(target)
>
> It seems like we should move these functions to `WI.TargetManager` so they
don't get out of sync between 'Main.js' and 'Test.js'.
Done.
>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:47
>> + auto target = create(provisionalPage.page(), targetId, type);
>
> NIT: I think for the purposes of code searching, I'd rather you also have the
class name here too:
> ```cpp
> auto target = InspectorTargetProxy::create(provisionalPage.page(),
targetId, type);
> ```
Done. But it'd be better to improve the tooling so that it understands such
calls :)
>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:65
>> if (m_page.hasRunningProcess())
>
> Style: missing newline (since the previous `if` has an early return)
Done.
>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:75
>> if (m_page.hasRunningProcess())
>
> Ditto
Done.
>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:85
>> if (m_page.hasRunningProcess())
>
> Ditto
Done.
>> Source/WebKit/UIProcess/InspectorTargetProxy.h:51
>> + bool isProvisional() const override;
>
> NIT: these could all be `final` instead of `override` (or even omitted, since
the class is marked `final`).
This contradicts webkit.org/b/200959#c13, let's agree on the approach to
override/final over there and I can update this patch accordingly. Omitting
override/final altogether causes clang warnings as I mentioned in that bug and
there is an outstanding patch generated via clang-tidy that fixes the issue
(see
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html#mode
rnize-use-override).
>> Source/WebKit/UIProcess/InspectorTargetProxy.h:62
>> + ProvisionalPageProxy* m_provisionalPage = nullptr;
>
> Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then
we could simply `m_provisionalPage.reset()` instead of assigning it to
`nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`.
>
> Style: please use uniform initialization syntax
> ```cpp
> ProvisionalPageProxy* m_provisionalPage { nullptr };
> ```
Updated the initializer.
I believe you meant Optional<ProvisionalPageProxy&> as this class doesn't own
ProvisionalPageProxy. What is the advantage of using more verbose
Optional<ProvisionalPageProxy&> than a raw pointer here? WebKit style guide
doesn't provide any specific recommendations about it.
>> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:96
>> + m_page.inspectorController().didDestroyProvisionalPage(*this,
m_wasCommitted);
>
> Rather than go through the trouble of telling Web Inspector about this even
when `m_wasCommitted` (which we then use to immediately return inside
`WebPageInspectorController::didDestroyProvisionalPage`), can we just move this
call below the early-return `if` below?
>
> Also, should this be the first call inside this function? That's usually
where we normally put these types of lifecycle things (the `didCreate*` is the
last line and the `willDestroy*` is the first line).
I tried to keep create/destroy notifications balanced no regardless of the
provisional navigation outcome. That's the reason didDestroyProvisionalPage was
before 'if (m_wasCommitted)' check. Updated the patch to only call
didDestroyProvisionalPage for not committed pages.
>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:67
>> + WebPageProxy& page() const { return m_page; }
>
> Why was this changed? It seems like it's because
`WebPageInspectorController::didDestroyProvisionalPage` requires a `const
WebCore::PageIdentifier&`, but is there a reason why it has to be that way?
Can you just pass it as a non-`const` reference?
This is exactly as you described. The argument of didDestroyProvisionalPage is
marked const as I'd like to indicate that it does't change its parameter. More
generally marking a method as const if it's known to not change the instance's
state gives the compiler more room for optimization.
>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:61
>> + createInspectorTarget(pageTargetId,
Inspector::InspectorTargetType::Page);
>
> I think we still want this to be at the very end of
`WebPageProxy::WebPageProxy`, to ensure that we're not trying to access any
uninitialized data.
This is how this code was already written I'm not changing it. If your goal is
to protect against access to a partially initialized instance of WebPageProxy
it should not be passed to the WebPageInspectorController until it's
sufficiently initialized. Do you you prefer moving WebPageInspectorController
call from member initialization in WebPageProxy to the end of the constructor
or introducing separate method WebPageInspectorController::init(WebPageProxy&
page) ? The latter means that the member pointing to WebPageProxy cannot be a
reference anymore.
>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:194
>> + // We've disconnected from the old page and will not receive any
message from it, so
>
> Style: missing newline
Done.
>> Source/WebKit/UIProcess/WebPageInspectorController.h:69
>> + void didDestroyProvisionalPage(const ProvisionalPageProxy&, bool
wasCommitted);
>
> Our normal "lifecycle" function names are `didCreate*` and `willDestroy*`.
Technically, when `didDestroyProvisionalPage` is called, the
`ProvisionalPageProxy` hasn't actually been destructed yet, so it's not
entirely accurate to say that we "did" destroy it.
Done.
>> Source/WebKit/UIProcess/WebPageInspectorController.h:71
>> + void didCommitProvisionalPage(const WebCore::PageIdentifier&
oldWebPageID, const WebCore::PageIdentifier& newWebPageID);
>
> These probably shouldn't be references, as `PageIdentifier` is basically just
a `uint64_t`. Other cases of `PageIdentifier` as a parameter also are not
references.
Yeah, it's used as a value type. Updated.
>> Source/WebKit/UIProcess/WebPageInspectorController.h:81
>> + HashMap<String, std::unique_ptr<InspectorTargetProxy>> m_targets;
>
> Could we use `WTF::UniqueRef` instead? I don't think we ever want a
situation where we'd have a `nullptr` value in this map.
I can't seem to be able to use UniqueRef as a type of HashMap values because
emptyValue() is not defined for it:
DerivedSources/ForwardingHeaders/wtf/HashTraits.h:67:36: error: no matching
constructor for initialization of
'WTF::UniqueRef<WebKit::InspectorTargetProxy>'
static T emptyValue() { return T(); }
^
DerivedSources/ForwardingHeaders/wtf/HashTraits.h:78:55: note: in instantiation
of member function
'WTF::GenericHashTraits<WTF::UniqueRef<WebKit::InspectorTargetProxy>
>::emptyValue' requested here
new (NotNull, std::addressof(slot)) T(Traits::emptyValue());
Also I don't see any examples in the code where UniqueRef would be used to
store map values.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:2983
>> + const auto oldWebPageID = m_webPageID;
>
> Ditto (+WebPageInspectorController.h:71), specifically to the drop the
`const`.
This is different as it's not a reference here. I'd like to make sure
oldWebPageID is not modified accidentally before it is passed to the inspector
controller.
>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:50
>> + if (m_channel)
>
> Should we `ASSERT(!m_channel);` to make sure that we're not trying to connect
to a target more than once?
I believe in the current implementation it's possible that connect() is called
more than once with different front-end types.
>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:72
>> +String WebPageInspectorTarget::toTargetID(const WebCore::PageIdentifier&
pageID)
>
> Ditto (+WebPageInspectorController.h:71)
Done.
>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41
>> + ~WebPageInspectorTargetFrontendChannel() override = default;
>
> `virtual ~WebPageInspectorTargetFrontendChannel() = default;`
Why? It overrides virtual destructor in its superclass.
>> LayoutTests/ChangeLog:13
>> + *
http/tests/inspector/target/target-events-for-proisional-page.html: Added.
>
> Typo: "proisional" => "provisional"
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expec
ted.txt:8
>> +PASS: received targetCreated event.
>
> We try to phrase our test messages as a "Should ___." (e.g. `Should receive
`targetCreated` event."), as well as using complete sentences.
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expec
ted.txt:9
>> +PASS: target is provisional.
>
> e.g. "Target should be provisional."
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:
10
>> + suite.addTestCase({
>
> Style: missing newline
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:
14
>> + const targetAgent = InspectorBackend.domains.Target;
>
> `InspectorBackend.domains` exists only for feature checking purposes. It's
not meant to be used for actual invocations of commands.
>
> I think a clearer approach to this would be to instead make your own
`TestPSONTargetManager` that extends from `WI.TargetManager` and just overrides
the functions you care about.
> ```js
> class TestPSONTargetManager extends TargetManager {
> targetCreated(targetInfo) {
> newTargetId = targetInfo.targetId;
> InspectorTest.pass(`received ${prop} event.`);
> InspectorTest.expectTrue(targetInfo.isProvisional, "target is
provisional.");
> InspectorTest.expectEqual(targetInfo.previousTargetId,
mainTargetId, "previos target id is id of the current target.");
>
> super.targetCreated(targetInfo);
> }
>
> didCommitProvisionalTarget(previousTargetId, targetInfo) {
> InspectorTest.pass(`received ${prop} event.`);
> InspectorTest.expectEqual(previousTargetId, mainTargetId,
"previous target is the current one.");
> InspectorTest.expectEqual(targetInfo.targetId, newTargetId,
"committed target matches provisional target.");
>
> super.targetCreated(targetInfo);
> }
>
> targetDestroyed(targetId) {
> InspectorTest.pass(`received ${prop} event.`);
> InspectorTest.expectEqual(targetId, mainTargetId, "destroyed
target is previos target.");
>
> super.targetCreated(targetInfo);
> }
> }
>
> let testPSONTargetManager = new TestPSONTargetManager;
> WI.managers.splice(WI.managers.indexOf(WI.targetManager), 1,
testPSONTargetManager);
> WI.targetManager = new TestPSONTargetManager;
> ```
>
> This still feels quite "hacky", but it's not a bad solution. Another option
is to just modify the prototype functions directly (e.g.
`WI.TargetManager.prototype.targetCreated`), or to add special event dispatches
that are only fired for tests.
I don't like the idea of deriving from TargetManager which is called indirectly
when dispatching protocol commands, I'd like to be as close as possible to the
target domain handler. Ideally this would be a protocol test if it didn't
depend on the dummy page and supported target domain but this is a different
problem. I changed the code to use TargetAgent similar to how other tests use
*Agents. I hope this will be changed to a clean protocol test eventually.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:
25
>> +
InspectorTest.expectEqual(targetInfo.previousTargetId, mainTargetId, "previos
target id is id of the current target.");
>
> Typo: "previos" => "previous"
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:
40
>> + InspectorTest.expectEqual(targetId,
mainTargetId, "destroyed target is previos target.");
>
> Ditto
Done.
>>
LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:
61
>> + 3. Target.targetDestroyed for the old target after the navigation request
is committed.
>
> Please make these into separate `<p>` so it is printed somewhat nicely in the
expected results file.
Done.
>> LayoutTests/platform/wk2/TestExpectations:767
>> +# Target domain is only present in WebKit2.
>
> NIT: just `WK2` is fine :)
Most of the other comments in this file use WebKit2 so I'd rather be
consistent.
More information about the webkit-reviews
mailing list