[webkit-reviews] review denied: [Bug 43683] Implement iframe.srcdoc : [Attachment 65877] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Justin Schuh <jschuh at chromium.org>'s
request for review:
Bug 43683: Implement iframe.srcdoc
https://bugs.webkit.org/show_bug.cgi?id=43683

Attachment 65877: Patch
https://bugs.webkit.org/attachment.cgi?id=65877&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (attr->name() == srcAttr) {
> +	   if (!hasTagName(iframeTag) || getAttribute(srcdocAttr).isNull())
> +	       setLocation(deprecatedParseURL(attr->value()));

It's bad layering to have iframe-specific code in HTMLFrameElementBase and then
check hasTagName(iframeTag). Instead, iframe-specific code should go in
HTMLIFrameElement. If we need a hook for iframe, we can add a virtual function,
but it should ask a question, not "is this an iframe".

To check if an attribute exists, we should use hasAttribute, not
getAttribute().isNull(). It's more efficient. But also, is null the only value
that matters? I'd like to see test cases covering values like empty string or
all whitespace for the srcdoc attribute.

Code like this that works with one attribute looking at the value of the other
usually is a problem because there's no guarantee the src attribute will be
parsed before the srcdoc attribute. I need to see how this will work in either
order, and we'd want to make sure we had test cases that covered both.

>  KURL HTMLFrameElementBase::location() const
>  {
> +    if (hasTagName(iframeTag) && !getAttribute(srcdocAttr).isEmpty())
> +	   return srcdocURL();
>      return document()->completeURL(getAttribute(srcAttr));
>  }

Again, not good to have iframe-specific code in HTMLFrameElementBase. Should be
factored differently. Same comment about getAttribute(srcdocAttr) too.

> +    if (str == srcdocURL().string())
> +	   return;

Is it OK that this is a case-sensitive comparison? I would like to see test
cases with "ABOUT:SRCDOC" too.

> +void HTMLFrameElementBase::setLocationToSrcdoc()
> +{
> +    m_URL = srcdocURL().string();
> +
> +    if (inDocument())
> +	   openURL(false, false);
> +}

It's bad for long term maintenance to copy and paste the last part of
setLocation into this separate function. Even though it's just a single
function call, I'd like to factor things so the code is shared.

> +SubstituteData HTMLFrameOwnerElement::srcdocSubstituteData(const KURL& url)
const
> +{
> +    if (url == srcdocURL() && hasTagName(HTMLNames::iframeTag)) {
> +	   const String srcdoc = getAttribute(HTMLNames::srcdocAttr);
> +	   if (!srcdoc.isNull())
> +	       return SubstituteData(srcdoc, document()->isXHTMLDocument() ?
"text/xml" : "text/html", KURL(), url);
> +    }
> +    return SubstituteData();
> +}

It's not appropriate to have the frame owner element have this logic that's
specific to iframe. Just as with HTMLFrameElementBase, this needs to be based
on virtual functions, not "is iframe" checks in the base class.

The result of getAttribute is a const AtomicString&, not a String. Putting it
into a String causes a bit of unnecessary reference count churn.

> +	   // Navigate using @src if @srcdoc is getting cleared.

The use of "@" here is not normal for comments in Webit.

> +	   if (attr->isNull())
> +	       setLocation(getAttribute(srcAttr));
> +	   else {
> +#if ENABLE(SANDBOX)
> +	       SandboxFlags flags = sandboxFlags();
> +	       if (flags == SandboxNone)
> +		   setSandboxFlags(SandboxAll);
> +#endif
> +	       setLocationToSrcdoc();
> +	   }

It's important that this logic be localized in a single class. We don't want
this part in one class and the rest in another. While other classes can forward
things here, the logic specific to the srcdoc attribute should be localized all
in a single class, HTMLIFrameElement. The other classes should not even know
the name of the attribute.

Also note that you can call fastGetAttribute in this case.

It's a factoring warning sigh that this needs its own extra sandboxing logic.
Is there any way to share this with the normal code path? Also, this is good
place for a "why" comment. I see what is happening to the sandbox flags, but
not why, and I am not the only one.

I don't see any point to putting the flags into a local variable here.

> +	   attribute [Reflect] DOMString srcdoc;

If the srcdoc attribute is a URL, then it needs to be "[Reflect, URL]" and
needs to be covered by the URL reflection test, and you'll also need to
override isURLAttribute to pass that test.

> +	   Frame* realParentFrame = m_frame->tree()->parent();
> +	   if (realParentFrame) {
> +	       // Walk a srcdoc hierarchy to a parent with a real encoding
> +	       const HTMLFrameOwnerElement* owner =
realParentFrame->ownerElement();
> +	       while (owner && owner->hasTagName(HTMLNames::iframeTag) &&
owner->hasAttribute(HTMLNames::srcdocAttr)) {
> +		   realParentFrame = realParentFrame->tree()->parent();
> +		   owner = realParentFrame ? realParentFrame->ownerElement() :
0;
> +	       }
> +	   }

This logic should go into a separate helper function. Adding it in to
DocumentWriter::createDecoderIfNeeded is not good factoring. Further, why is
determining the encoding the one place where we need this "real parent" logic
that skips srcdoc frames? What other calls to parent() might want this? Maybe
FrameTree itself should know about this. Again, the logic should not check
iframeTag and srcdocAttr. It should use a virtual function -- only the
HTMLIFrameElement class itself should know about srcdocAttr.

> -    RefPtr<DocumentLoader> loader = m_client->createDocumentLoader(request,
SubstituteData());
> +    SubstituteData substituteData = m_frame->ownerElement() ?
m_frame->ownerElement()->srcdocSubstituteData(request.url()) :
SubstituteData();
> +    RefPtr<DocumentLoader> loader = m_client->createDocumentLoader(request,
substituteData);

The way it's used here makes it clear that srcdocSubstituteData is not the
right name for this function. While the frame only uses this when a srcdoc is
involved, the actual name should simply state that this is SubstituteData to be
used when loading the frame.

> -    if (!SecurityOrigin::canLoad(url, referrer, 0)) {
> +    if (!SecurityOrigin::canLoad(url, referrer, ownerElement->document())) {


Why this change? What test covers it? Can this go in by itself?

> -	   SubstituteData(PassRefPtr<SharedBuffer> content, const String&
mimeType, const String& textEncoding, const KURL& failingURL, const KURL&
responseURL = KURL())
> -	       : m_content(content)
> -	       , m_mimeType(mimeType)
> +	   
> +	   SubstituteData(PassRefPtr<SharedBuffer> dataBuffer, const String&
mimeType, const String& textEncoding, const KURL& failingURL, const KURL&
responseURL = KURL())

The term "data buffer" here doesn't seem very good. I think just "buffer" is
probably OK given the context.

> +	       : m_mimeType(mimeType)
>	       , m_textEncoding(textEncoding)
>	       , m_failingURL(failingURL)
>	       , m_responseURL(responseURL)
>	   {
> +	       m_content = adoptRef(new SubstituteContentBuffer(dataBuffer));
>	   }

Why is m_content being set up in an assignment instead of initialization? It
should be initialization. It's OK to call a function in an initializer.

In new code, you should be using a create function, not a direct call to
adoptRef(new).

> +#if defined(WTF_CPU_BIG_ENDIAN) || defined(WTF_CPU_MIDDLE_ENDIAN)
> +	       , m_textEncoding("UTF-16BE")
> +#else
> +	       , m_textEncoding("UTF-16LE")
> +#endif

This is really weak way to do this. Can we find a cleaner way to accomplish it?
I think we might want to offer a function that returns a suitable TextEncoding
for UChar without looking up the encoding name as a string each time.

Also, the WTF_XXX macros aren't intended to be used directly with defined. If
you were going to do the #if it would be #if CPU(BIG_ENDIAN). But there has to
be a better way to handle it.

> +	       m_content = adoptRef(new SubstituteContentString(dataString));

Same comments as above about assignment, adoptRef, and create.

> +	       virtual ~SubstituteContent() {};

No semicolon needed here. Also, we put a space between braces in cases like
this.

> +	       SubstituteContentBuffer(PassRefPtr<SharedBuffer> dataBuffer) :
m_dataBuffer(dataBuffer) {}

Put space between the braces in cases like this.

> +	       const char* data() const { return m_dataBuffer->data(); }
> +	       unsigned size() const { return m_dataBuffer->size(); }

Please make these private and mark them virtual. We like to be explicit when we
are overriding virtual functions and repeat the virtual, and it would be
inefficient for someone to call these virtual functions if they knew the
concrete type; making the functions private makes it more likely we'd catch
that mistake.

The word data in m_dataBuffer doesn't add much.

Is unsigned OK for size? Elsewhere we often use size_t for such sizes.

> +	       const char* data() const { return reinterpret_cast<const char
*>(m_dataString.characters()); }

There should not be a space before the "*". This needs a comment connecting
this otherwise-unwise cast to the text encoding names that make it OK.

> +	   private:
> +	       String m_dataString;
> +	   };

The word "data" in m_dataString doesn't add much.

> +	   
>      };
> -
>  }
>  
>  #endif // SubstituteData_h

Please don't remove that blank line. We want a blank line there.

Do the test cases cover all the code changes here?

review- because you'll want to fix at least some of the things I mentioned
above