[webkit-reviews] review granted: [Bug 200364] Web Inspector: "Inspector.initialized" happens before breakpoints are set : [Attachment 375370] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 2 12:36:19 PDT 2019
Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200364: Web Inspector: "Inspector.initialized" happens before breakpoints
are set
https://bugs.webkit.org/show_bug.cgi?id=200364
Attachment 375370: Patch
https://bugs.webkit.org/attachment.cgi?id=375370&action=review
--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375370
--> https://bugs.webkit.org/attachment.cgi?id=375370
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=375370&action=review
> Source/WebInspectorUI/UserInterface/Protocol/Target.js:89
> + Promise.all(Target._initializationPromises).then(() => {
Would we want to assert here that `Target._initializationPromises` is not
empty? We always expect at least one (Breakpoints)
This assert would let us know if we change something and went too early.
> Source/WebInspectorUI/UserInterface/Protocol/Target.js:145
> + promise.then(() => {
> + Target._initializationPromises.remove(promise);
> + });
Oh, you're removing promises from the list... probably to reduce memory.
Maybe we should leave them for the assert, which seems pretty valuable. Or have
an assertion:
console.assert((Target._initializationPromises.length > 0) ||
(Target._completedInitializationPromises > 0));
And have this increment `Target._completedInitializationPromises` as well as
remove the promise.
That keeps removing the promises at the cost of a counter for a useful
assertion.
More information about the webkit-reviews
mailing list