Closed
Bug 1024447
Opened 9 years ago
Closed 9 years ago
Generating HTML parser using translator causes build error
Categories
(Core :: DOM: HTML Parser, defect)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: yuki.sekiguchi, Assigned: yuki.sekiguchi)
Details
Attachments
(7 files)
3.87 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
4.16 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
3.59 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
51.21 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•9 years ago
|
||
If my expectation is correct, I already fixed this bug locally. I'll attach the patch soon.
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
Looks like the patch from bug 870021 didn't get pushed to the htmlparser repo.
Updated•9 years ago
|
Attachment #8439853 -
Flags: review?(hsivonen) → review+
Comment 10•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8439859 -
Flags: review?(hsivonen) → review+
Updated•9 years ago
|
Attachment #8439861 -
Flags: review?(hsivonen) → review+
Updated•9 years ago
|
Attachment #8439866 -
Flags: review?(hsivonen) → review+
Comment 11•9 years ago
|
||
(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
Updated•9 years ago
|
Assignee: nobody → yuki.sekiguchi
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
(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)
Comment 14•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8439858 -
Flags: review?(hsivonen) → review+
Updated•9 years ago
|
Attachment #8439868 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Hi Henri, Thank you for reviewing. Could you commit my patches?
Comment 16•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 19•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8442537 -
Flags: review?(hsivonen) → review+
Comment 20•9 years ago
|
||
Thanks. Landed: https://hg.mozilla.org/projects/htmlparser/rev/3344e4f41d7e https://hg.mozilla.org/integration/mozilla-inbound/rev/f40a6e9e4596
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f40a6e9e4596
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 22•9 years ago
|
||
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.
Description
•