Closed Bug 1583348 Opened 4 years ago Closed 4 years ago

Write a gTest for mozilla::dom::DOMParser::CreateWithoutGlobal()

Categories

(Core :: DOM: Core & HTML, task)

Product:
Core
Component:
DOM: Core & HTML
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 2 obsolete files)

M-C version: 1583348-gtest-parser.patch (v2)
2.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review

M-C provides the mozilla::dom::DOMParser::CreateWithoutGlobal() function for us, but it's not tested anywhere, see bug 1454842 comment #13.

TB's processing of HTML mail relies on that function, see:
https://searchfox.org/comm-central/rev/30f32c40a88dc34746d22697dad8fe99c7922054/mailnews/mime/src/mimeTextHTMLParsed.cpp#83

I'm sure other tests would pick up a failure, but we should also test the function directly now that we have gTests in automation.

Comment on attachment 9099107 [details] [diff] [review]
C-C version: 1583348-gtest-parser.patch

Let's see whether we can stick the test into M-C. I'll prepare a patch for that.
Attachment #9099107 - Flags: review?(acelists)
Attachment #9099107 - Attachment description: 1583348-gtest-parser.patch → C-C version: 1583348-gtest-parser.patch

Hello Boris, this is a follow-up to bug 1454842 comment #11. Can we implement this test in M-C so we notice a problem before it's committed? mach gtest TestParser* runs the test successfully. Please let me know which changes you require. Or whether you prefer not to have it in M-C.

Attachment #9099234 - Flags: review?(bzbarsky)
Comment on attachment 9099234 [details] [diff] [review]
M-C version: 1583348-gtest-parser.patch

>+  nsString htmlInput;
>+  htmlInput.AssignLiteral(
>+      "<html><head>"
>+      "<meta http-equiv=\"content-type\" content=\"text/html; charset=\">"
>+      "</head><body>Hello <b>Thunderbird!</b></body></html>");

Any reason this is not NS_NAMED_LITERAL_STRING?

>+    RefPtr<mozilla::dom::DOMParser> parser =
>+        mozilla::dom::DOMParser::CreateWithoutGlobal(rv2);

If rv2 here fails, something needs to happen.  Probably break?

>+    nsCOMPtr<mozilla::dom::Document> document = parser->ParseFromString(
>+        htmlInput, mozilla::dom::SupportedType::Text_html, rv2);
>+    if (rv2.Failed()) break;

rv2 should be an IgnoredErrorResult; otherwise this will assert if the failure path is taken.

>+    printf("all tests passed\n");

Why?
Attachment #9099234 - Flags: review?(bzbarsky) → review+

Thanks, Boris, could you stick the r+ onto it one more time.
EDIT: https://bugzilla.mozilla.org/attachment.cgi?oldid=9099234&action=interdiff&newid=9099710&headers=1

The answer to all your questions is possibly lack of experience.

mach gtest TestParser* still passes.

Attachment #9099107 - Attachment is obsolete: true
Attachment #9099234 - Attachment is obsolete: true
Attachment #9099710 - Flags: review?(bzbarsky)
Attachment #9099710 - Flags: review?(bzbarsky) → review+

Dear sheriff, can you please land the attached patch the old-fashioned way.

Please import the patch with
hg qimport bz:1583348
or
hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=9099710 -n 1583348.patch
to avoid changing the author.

Component: General → DOM: Core & HTML
Keywords: checkin-needed
Product: MailNews Core → Core
Version: unspecified → Trunk

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ec88fbbfcf8
Implement gTest for mozilla::dom::DOMParser::CreateWithoutGlobal(). r=bz

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/7ec88fbbfcf8

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.