Summary: | Rendered <noscript> tag if XHTMLMP feature is enable. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Heedol Lee <heedol.lee> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Major | CC: | abarth, ap, commit-queue, eric, kyounga.ra, webkit.review.bot | ||||||||||||
Priority: | P1 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows XP | ||||||||||||||
URL: | http://news.google.com/nwshp?hl=en&tab=wn | ||||||||||||||
Attachments: |
|
Description
Heedol Lee
2010-10-27 19:10:43 PDT
Created attachment 72129 [details]
Render problem for noscript tag.
Even though script is enabled, the noscript element is inserted in HTMLTreeBuilder using processGenericRawTextStartTag(). It seems that the parser is supposed to support HTML5's requirement. Reference : http://www.whatwg.org/specs/web-apps/current-work/#the-noscript-element. So, I think it is right that the presenting issue is handed by rendererIsNeeded(). But, the noscript element must not be used in XML documents. The noscript element is only effective in the HTML syntax, it has no effect in the XHTML syntax. So, we should ignore noscript element or make it a parse error for XHTML, shall we? Created attachment 72783 [details]
If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not.
If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement.
I use this to determine if noscript should be rendered or not.
Comment on attachment 72783 [details] If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not. View in context: https://bugs.webkit.org/attachment.cgi?id=72783&action=review We need a ChangeLog and a test. > WebCore/html/HTMLElement.cpp:784 > + if (!document()->shouldProcessNoscriptElement()) What is this bit? How is it computed? Created attachment 73314 [details]
patch
If XHTMLMP is enabled, class "document" has an additional member, m_shouldProcessNoScriptElement. I use this to determine if noscript should be rendered or not.
The bit(m_shouldProcessNoScriptElement) is computed by default as following in Doucment class constructor.
#if ENABLE(XHTMLMP)
m_shouldProcessNoScriptElement = !(m_frame && m_frame->script()->canExecuteScripts(NotAboutToExecuteScript));
#endif
I mean it same as before.
and it can be set by calling setShouldProcessNoscriptElement().
It is called only in XMLDocumentParser.
Attachment 73314 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/html/HTMLElement.cpp']" exit_code: 1
WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 73314 [details] patch >Index: WebCore/ChangeLog >=================================================================== >--- WebCore/ChangeLog (revision 71478) >+++ WebCore/ChangeLog (working copy) >@@ -1,3 +1,13 @@ >+2010-11-07 kyounga <kyounga.ra@gmail.com> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ <noscript> is rendered with enabled XHTMLMP. >+ https://bugs.webkit.org/show_bug.cgi?id=48493 >+ >+ * html/HTMLElement.cpp: >+ (WebCore::HTMLElement::rendererIsNeeded): >+ > 2010-11-06 Pavel Feldman <pfeldman@chromium.org> > > Reviewed by Simon Fraser. >Index: WebCore/html/HTMLElement.cpp >=================================================================== >--- WebCore/html/HTMLElement.cpp (revision 71020) >+++ WebCore/html/HTMLElement.cpp (working copy) >@@ -778,14 +778,16 @@ PassRefPtr<HTMLCollection> HTMLElement:: > > bool HTMLElement::rendererIsNeeded(RenderStyle *style) > { >-#if !ENABLE(XHTMLMP) > if (hasLocalName(noscriptTag)) { > Frame* frame = document()->frame(); >+#if ENABLE(XHTMLMP) >+ if (!document()->shouldProcessNoscriptElement()) >+ return false; >+#else > if (frame && frame->script()->canExecuteScripts(NotAboutToExecuteScript)) > return false; >- } else >-#endif >- if (hasLocalName(noembedTag)) { >+#endif >+ } else if (hasLocalName(noembedTag)) { > Frame* frame = document()->frame(); > if (frame && frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)) > return false; > Created attachment 73321 [details]
patch - including ChangeLog and ChangeLog's style
tab is removed in changeLog
(In reply to comment #4) > (From update of attachment 72783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72783&action=review > > We need a ChangeLog and a test. Sorry I missed it. I attached the patch including ChangeLog. I tested the reported url with XHTMLMP enabled. In case of disabled XHTMLMP, the code is same. > > > WebCore/html/HTMLElement.cpp:784 > > + if (!document()->shouldProcessNoscriptElement()) > > What is this bit? How is it computed? I added a comment #5. The bit(m_shouldProcessNoScriptElement) is computed by default as following in Doucment class constructor. #if ENABLE(XHTMLMP) m_shouldProcessNoScriptElement = !(m_frame && m_frame->script()->canExecuteScripts(NotAboutToExecuteScript)); #endif It can be set by calling setShouldProcessNoscriptElement() called only in XMLDocumentParser. Comment on attachment 73321 [details]
patch - including ChangeLog and ChangeLog's style
Looks OK.
Comment on attachment 73321 [details] patch - including ChangeLog and ChangeLog's style View in context: https://bugs.webkit.org/attachment.cgi?id=73321&action=review Please post a new patch with a fixed ChangeLog, or get someone to fix the patch when landing this for you. > WebCore/ChangeLog:1 > +2010-11-07 kyounga <kyounga.ra@gmail.com> Your ChangeLog entry should have your full name. Created attachment 76515 [details]
Including my full name in ChangeLog
I posted a new patch only to leave my full name in ChangeLog and set the review flag to [+] because this solution was reviewd by Eric.
Is it OK?
If I should re-request for review, let me know. anyone please.
Comment on attachment 76515 [details]
Including my full name in ChangeLog
A reviewer needs to set the review flag to + for the tools to land the change.
Comment on attachment 76515 [details] Including my full name in ChangeLog Clearing flags on attachment: 76515 Committed r74059: <http://trac.webkit.org/changeset/74059> |