Simple parsing of pseudo-attributes in processing instruction [chromium/src : main]

0 views

Skip to first unread message

Philip Jägenstedt (Gerrit)

unread,

11:27 AM (12 hours ago) 11:27 AM

to Noam Rosenthal, chromium...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Noam Rosenthal

Philip Jägenstedt added 4 comments

File third_party/blink/renderer/core/dom/processing_instruction.cc

Line 128, Patchset 8 (Latest): fake_html.Append("<attrib ");

Philip Jägenstedt . unresolved

How about using <attrs>, the same as the XML code path?

Line 147, Patchset 8 (Latest):String ProcessingInstruction::GetAttribute(const String& name) {

Philip Jägenstedt . unresolved

Missing newline.

Line 149, Patchset 8 (Latest): return attributes_.Contains(name) ? attributes_.at(name) : String("");

Philip Jägenstedt . unresolved

Can we use `attributes_.find(name)` here instead so that the cost of lookup is only paid once?

File third_party/blink/renderer/core/html/parser/html_document_parser_test.cc

Line 266, Patchset 8 (Latest): EXPECT_EQ("", pi->GetAttribute("value"));

Philip Jägenstedt . unresolved

The web-exposed `getAttribute()` returns null if there's no such attribute to distinguish between missing attribute and empty value. Can we do the same?

Open in Gerrit

Related details

Attention is currently required from:

Noam Rosenthal

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Noam Rosenthal (Gerrit)

unread,

12:53 PM (11 hours ago) 12:53 PM

to Philip Jägenstedt, chromium...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Philip Jägenstedt

Noam Rosenthal added 4 comments

File third_party/blink/renderer/core/dom/processing_instruction.cc

Line 128, Patchset 8: fake_html.Append("<attrib ");

Philip Jägenstedt . resolved

How about using <attrs>, the same as the XML code path?

Noam Rosenthal

Done

Line 147, Patchset 8:String ProcessingInstruction::GetAttribute(const String& name) {

Philip Jägenstedt . resolved

Missing newline.

Noam Rosenthal

Done

Line 149, Patchset 8: return attributes_.Contains(name) ? attributes_.at(name) : String("");

Philip Jägenstedt . resolved

Can we use `attributes_.find(name)` here instead so that the cost of lookup is only paid once?

Noam Rosenthal

Done

File third_party/blink/renderer/core/html/parser/html_document_parser_test.cc

Line 266, Patchset 8: EXPECT_EQ("", pi->GetAttribute("value"));

Philip Jägenstedt . resolved

The web-exposed `getAttribute()` returns null if there's no such attribute to distinguish between missing attribute and empty value. Can we do the same?

Noam Rosenthal

Done

Open in Gerrit

Related details

Attention is currently required from:

Philip Jägenstedt

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages