Closed
Bug 1385907
Opened 7 years ago
Closed 7 years ago
nsHtml5SpeculativeLoad got bloated up a ton without review
Categories
(Core :: DOM: HTML Parser, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla57
| Tracking | Status | |
|---|---|---|
| firefox57 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jdm)
References
Details
Attachments
(1 file, 1 obsolete file)
|
17.81 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
Bug 1067345 got landed without any review and adds a ton of members to nsHtml5SpeculativeLoad. It didn't have to add this many members; it could have reused some existing ones. This should probably get at least reviewed, if not fixed.
Flags: needinfo?(josh)
Flags: needinfo?(hsivonen)
|
Assignee | |
Comment 1•7 years ago
|
||
Henri, do you want to review https://bugzilla.mozilla.org/attachment.cgi?id=8534778&action=diff in particular, or should I tag someone else (wchen, I guess)?
Flags: needinfo?(josh)
|
|
Assignee | |
Comment 2•7 years ago
|
||
Hmm, looks like the actual merged changeset did not exactly match the one that was attached to the bug, either: https://hg.mozilla.org/integration/mozilla-inbound/rev/b64c085b06e1 . In particular, I apparently cleaned up the sketchy changes in HtmlSpeculativeLoad::Perform.
|
||
Comment 3•7 years ago
|
||
The total number of nsString members of nsHtml5SpeculativeLoad should not exceed the number of strings transported by the speculative load type that needs the largest number of strings. AFAICT, the max is 5 but we have 10 nsString members. We should have only 5 nsString members whose semantics depend on the op code.
Flags: needinfo?(hsivonen)
|
|
||
Comment 4•7 years ago
|
||
(The changeset looked ok otherwise.)
|
||
Updated•7 years ago
|
Priority: -- → P2
|
|
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8898067 -
Flags: review?(hsivonen)
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → josh
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 6•7 years ago
|
||
Attachment #8898081 -
Flags: review?(hsivonen)
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8898067 -
Attachment is obsolete: true
Attachment #8898067 -
Flags: review?(hsivonen)
|
|
Assignee | |
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=884475e4233c14452bc2bffdbf42694c29cafd01
|
|
||
Comment 8•7 years ago
|
||
Comment on attachment 8898081 [details] [diff] [review] Combine exclusive fields of nsHtml5SpeculativeLoad Review of attachment 8898081 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.
Attachment #8898081 -
Flags: review?(hsivonen) → review+
|
|
Assignee | |
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/72e90a73a9bf Combine exclusive fields of nsHtml5SpeculativeLoad. r=hsivonen
Keywords: checkin-needed
|
||
Comment 10•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/72e90a73a9bf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•