Closed
Bug 1220936
Opened 9 years ago
Closed 8 years ago
Consider using nsContentUtils::GetWindowID() when flushing ConsoleReportCollectors
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bkelly, Assigned: tt)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
Currently the ConsoleReportCollector is flushed to a specific nsIDocument in either the channel or in docshell. An alternative, would be to use nsContentUtils::GetWindowID() instead. This method uses the load group to find the associated window instead.
The advantage of the load group approach is it will report the message to the correct console even if the network request was initiated from a worker without a loading document.
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 2•8 years ago
|
||
Sorry about late. I was working on storage API.
Initial patch for this bug.
I listed my changes below inline:
- Add "InnerWindowID" into ReportToConsole() and ReportToConsoleNonLocalized() to allow using window ID directly when we don't have a document.
- Rewrite FlushReportsByWindowID and FlushConsoleReports() since they are simlar.
Next: thinking about maybe create a method that flush report into loadGroup since both document(GetDocumentLoadGeoup()) and worker(GetLoadGroup) can get loadgroup easily
Assignee | ||
Comment 3•8 years ago
|
||
Hi Ben,
In this patch, I intend to flush report based on window ID rather nsIDocument. However, I'm not so sure about the approach is suitable. Could you help me to review this patch? Thanks!
To do this, I list the things I did as following:
- Rewrite FlushConsoleReports(doc) and change its name to FlushReportsToConsole(id) since I think it's better to provide one way to flush rather than two at the same time, and I want to avoid being ambiguous with FlushConsoleReports(nsIConsoleReportCollector* aCollector) (e.g. FlushConsoleReports(0)).
- Remove FlushReportByWindowid() since it's redundant.
- Split the ReportToConsoleNonLocalized() into two pieces and name later one as ReportToConsoleByWindowID() to flush console report via window ID.
- Split the GetInnerWindowID() into two pieces since sometimes we have a loadGroup instead of a request.
- Get inner window ID from loadGroup/nsIRequest rather than document at first when calling FlushReportsToConsole(). If we don't have a loadgroup/request, it will follow the original logic to get ID from the document.
Attachment #8827837 -
Attachment is obsolete: true
Attachment #8830652 -
Flags: review?(bkelly)
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8830652 [details] [diff] [review]
Flush console reports to innerWindowID rather than nsIDocument.
Review of attachment 8830652 [details] [diff] [review]:
-----------------------------------------------------------------
Overall this looks good. Thanks!
I think we could just avoid a bit of code duplication with a few extra convenience methods. r=me with that change.
::: dom/console/ConsoleReportCollector.h
@@ +27,5 @@
> const nsTArray<nsString>& aStringParams) override;
>
> void
> + FlushReportsToConsole(uint64_t aInnerWindowID,
> + ReportAction aAction = ReportAction::Forget) override;
Can you please add a FlushReportsToConsole() that takes nsILoadGroup* and does the nsContentUtils::GetInnerWindowID() automatically? It seems we duplicate that code around a lot here.
Also perhaps there should be a FlushReportsToConsole() that takes nsIDocument* and does the `doc ? doc->InnerWindowID() : 0` logic.
Attachment #8830652 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 5•8 years ago
|
||
Addressed comment, but I'm a bit afraid I did somethings wrong here.
I list the changes in this patch as follow:
- Create two functions (FlushConsoleReports(doc) and FlushConsoleReports(loadGroup)) to handle the redundant logic. I avoid naming these two functions from FlushReportToConsole because it may be ambiguous when FlushReportToConsole(0).
- Limit FlushConsoleReports(doc) be called in MainThread. However, FlushConsoleReports(loadGroup) can be called in any thread since it supposes to be called anywhere even workers.
Since I did many changes in this patch, could you help me to review it one more time, Ben? I'll provide the inter-diff patch later. Thanks!
Attachment #8830652 -
Attachment is obsolete: true
Attachment #8832823 -
Flags: review?(bkelly)
Assignee | ||
Comment 6•8 years ago
|
||
inter-diff patch
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8832823 [details] [diff] [review]
Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument and nsILoadGroup.
Thanks!
Attachment #8832823 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8832823 [details] [diff] [review]
> Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument
> and nsILoadGroup.
>
> Thanks!
Thanks for the review!
Attachment #8832823 -
Attachment is obsolete: true
Attachment #8832824 -
Attachment is obsolete: true
Attachment #8833870 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ed23eb2f3
Flush console report to innerWindowID by using nsIDocument and nsILoadGroup. r=bkelly.
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•