Closed Bug 1024447 Opened 5 years ago Closed 5 years ago

Generating HTML parser using translator causes build error

Categories

(Core :: HTML: Parser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: yuki.sekiguchi, Assigned: yuki.sekiguchi)

Details

Attachments

(7 files)

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)
If my expectation is correct, I already fixed this bug locally. I'll attach the patch soon.
The following bug added srcset attribute, but it doesn't add the srcset to htmlparser.
https://bugzilla.mozilla.org/show_bug.cgi?id=870021
Attachment #8439853 - Flags: review?(hsivonen)
The following bug added keysystem atom, but it doesn't add the keysystem to htmlparser.
https://bugzilla.mozilla.org/show_bug.cgi?id=1016162
Attachment #8439858 - Flags: review?(hsivonen)
The following bug changed include guard name, but it didn't change translator code.
https://bugzilla.mozilla.org/show_bug.cgi?id=528863
Attachment #8439859 - Flags: review?(hsivonen)
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
Attachment #8439861 - Flags: review?(hsivonen)
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
Attachment #8439866 - Flags: review?(hsivonen)
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.
Attachment #8439868 - Flags: review?(hsivonen)
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.
Looks like the patch from bug 870021 didn't get pushed to the htmlparser repo.
Attachment #8439853 - Flags: review?(hsivonen) → review+
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?
Attachment #8439859 - Flags: review?(hsivonen) → review+
Attachment #8439861 - Flags: review?(hsivonen) → review+
Attachment #8439866 - Flags: review?(hsivonen) → review+
(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?
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → HTML: Parser
Ever confirmed: true
Flags: needinfo?(hsivonen)
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Assignee: nobody → 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.
(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.
Flags: needinfo?(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.
Attachment #8439858 - Flags: review?(hsivonen) → review+
Attachment #8439868 - Flags: review?(hsivonen) → review+
Hi Henri,

Thank you for reviewing. Could you commit my patches?
Sorry had to backout the mozilla-inbound part for bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=41950052&tree=Mozilla-Inbound
(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.
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.
Attachment #8442537 - Flags: review?(hsivonen)
Attachment #8442537 - Flags: review?(hsivonen) → review+
https://hg.mozilla.org/mozilla-central/rev/f40a6e9e4596
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Filed named character issue which was mentioned at Comment 7.
https://bugzilla.mozilla.org/show_bug.cgi?id=1029309
You need to log in before you can comment on or make changes to this bug.