1024447 - Generating HTML parser using translator causes build error

Status: RESOLVED FIXED

(Core :: HTML: Parser, defect)

Description

[Yuki Sekiguchi]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Steps to reproduce:

cd $MOZILLA_SRC/parser/html/java
make sync
make translate
make named_characters
cd $MOZILLA_SRC
./mach build


Actual results:

- build error because there is no include file mozilla/util.h
- srcset and keysystem attirute will be removed.
- int32_t is replaced with PRInt32.


Expected results:

- No build error.
- It should not change any source.(maybe)

Comment 1

[Yuki Sekiguchi]

If my expectation is correct, I already fixed this bug locally. I'll attach the patch soon.

Comment 2

[Yuki Sekiguchi]

Attachment: Part 1: Added srcset attribute

The following bug added srcset attribute, but it doesn't add the srcset to htmlparser.
https://bugzilla.mozilla.org/show_bug.cgi?id=870021

Comment 3

[Yuki Sekiguchi]

Attachment: Part 2: Added keysystem attribute.

The following bug added keysystem atom, but it doesn't add the keysystem to htmlparser.
https://bugzilla.mozilla.org/show_bug.cgi?id=1016162

Comment 4

[Yuki Sekiguchi]

Attachment: Part 3: Changed auto generated include guard from XXX_h_ to XXX_h.

The following bug changed include guard name, but it didn't change translator code.
https://bugzilla.mozilla.org/show_bug.cgi?id=528863

Comment 5

[Yuki Sekiguchi]

Attachment: Part 4: Translator should use stdint types, too.

The following bug changed gecko to use stdint type, but it didn't change translator code.
https://bugzilla.mozilla.org/show_bug.cgi?id=579517

Comment 6

[Yuki Sekiguchi]

Attachment: Part 5: Replace Util.h with ArrayUtils.h.

The following patch removed mozilla/Util.h, but translator uses it. It should use mozilla/ArrayUtils.h
https://bugzilla.mozilla.org/show_bug.cgi?id=713082

Comment 7

[Yuki Sekiguchi]

Attachment: Part 6: Retranslated html parser.

Retranslate html parser.
However, I cannot use "make named_characters" because it removes some named characters.
Recovering named character from array table is very tough for me, so I'll file another bug.
I checked "make named_characters" doesn't cause any build error.

Comment 8

[Yuki Sekiguchi]

Hi hsivonen,

Could you review my patches?
This is my first patch to mozilla, so I may make a mistake. Please correct me if I am wrong.

Comment 9

[Henri Sivonen (:hsivonen)]

Looks like the patch from bug 870021 didn't get pushed to the htmlparser repo.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8439858 [details] [diff] [review]
Part 2: Added keysystem attribute.

Where is this defined as a markup attribute? I see EME specifying it only as a DOM attribute.

Also, I don't see this on mozilla-central yet. Is this patch intentionally adding something that's not on m-c?

Comment 11

[John Schoenick [:johns]]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> Looks like the patch from bug 870021 didn't get pushed to the htmlparser
> repo.

There's another change in bug 870022 for <picture>, should I be opening upstream issues somewhere when these are landed?

Comment 12

[Yuki Sekiguchi]

Thank you for reviewing!

(In reply to Henri Sivonen (:hsivonen) from comment #10)
> Comment on attachment 8439858 [details] [diff] [review]
> Part 2: Added keysystem attribute.
> 
> Where is this defined as a markup attribute? I see EME specifying it only as
> a DOM attribute.

https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media.html#extensions
> The keysystem content attribute is added to HTMLSourceElement.

The spec refers to keysystem content attribute, but there is no the content attribute definition like srcset:
http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/#additions-to-the-img-element

IMHO, EME wants to add keysystem content attribute.

The original bug also reads and writes HTML attribute:
https://bugzilla.mozilla.org/attachment.cgi?id=8432966&action=diff#a/content/html/content/src/HTMLSourceElement.h_sec2

Therefore, I think we should add keysystem attribute to HTML parser.

> Also, I don't see this on mozilla-central yet. Is this patch intentionally
> adding something that's not on m-c?

There is keysystem on my repository which cloned using "hg clone https://hg.mozilla.org/mozilla-central/".
And, the mercurial web interface also has the atom https://hg.mozilla.org/mozilla-central/file/80431d4fd0da/parser/html/nsHtml5AtomList.h#l171
The getting code page says that mozilla-central is https://hg.mozilla.org/mozilla-central/:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial#mozilla-central_(main_development_tree)

Did I look at wrong place?
Sorry for novice question. I could not get the answer from #introduction.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to John Schoenick [:johns] from comment #11)
> (In reply to Henri Sivonen (:hsivonen) from comment #9)
> > Looks like the patch from bug 870021 didn't get pushed to the htmlparser
> > repo.
> 
> There's another change in bug 870022 for <picture>, should I be opening
> upstream issues somewhere when these are landed?

No need to open a separate upstream bug in order to push to the htmlparser repo. Please just prefix the bug number with "Mozilla bug " instead of "Bug " in the commit message to disambiguate what bug numbers are being referred to in htmlparser repo commits.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Yuki Sekiguchi from comment #12)
> https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-
> media.html#extensions
> > The keysystem content attribute is added to HTMLSourceElement.

OK.

> Did I look at wrong place?
> Sorry for novice question. I could not get the answer from #introduction.

Maybe I did.

Comment 15

[Yuki Sekiguchi]

Hi Henri,

Thank you for reviewing. Could you commit my patches?

Comment 16

[Henri Sivonen (:hsivonen)]

Landed. Thanks.
https://hg.mozilla.org/projects/htmlparser/rev/14192b7d7484
https://hg.mozilla.org/projects/htmlparser/rev/a2b5da497f83
https://hg.mozilla.org/projects/htmlparser/rev/227767d92f2c
https://hg.mozilla.org/projects/htmlparser/rev/969622611e65
https://hg.mozilla.org/projects/htmlparser/rev/82a52acf119c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5b73e6b782

(The sheriff will mark this FIXED when mozilla-inbound merges to mozilla-central.)

Comment 17

[Carsten Book [:Tomcat]]

Sorry had to backout the mozilla-inbound part for bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=41950052&tree=Mozilla-Inbound

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Carsten Book [:Tomcat] from comment #17)
> Sorry had to backout the mozilla-inbound part for bustages like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41950052&tree=Mozilla-
> Inbound

Hmm. I ran the translator locally for what was landed on m-c. Apparently that header include is still a discrepancy between what the translation yields and what's in m-c. Sorry about not testing it first.

Comment 19

[Yuki Sekiguchi]

Attachment: Part 7: Remove nsHtml5PendingNotification.h.

The following bug removed nsHtml5PendingNotification.h after this review was finished. Translator should also remove the include from translated code.
https://bugzilla.mozilla.org/show_bug.cgi?id=902618

This code doesn't affect to Part 6 diff.

Comment 20

[Henri Sivonen (:hsivonen)]

Thanks. Landed:
https://hg.mozilla.org/projects/htmlparser/rev/3344e4f41d7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40a6e9e4596

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f40a6e9e4596

Comment 22

[Yuki Sekiguchi]

Filed named character issue which was mentioned at Comment 7.
https://bugzilla.mozilla.org/show_bug.cgi?id=1029309