[Webkit-unassigned] [Bug 24150] Add virtual ScriptExecutionContext::encoding()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24150





------- Comment #9 from dimich at chromium.org  2009-03-04 18:01 PDT -------
(In reply to comment #6)
> (From update of attachment 27999 [review])
> > +    String inputEncoding() const { return encoding(); }
> > +    String charset() const { return encoding(); }
> > +    String characterSet() const { return encoding(); }
> 
> You need to use "return Document::encoding();" syntax to avoid virtual
> dispatch. 

Done.

> Please also consider making encoding() private (that would improve
> safety, but decrease clarity somewhat, as the functions won't be near each
> other any more).

Moved the function in .h file (to another similar one, userAgent()) but not
sure if making it private (and virtual) will help  - because on WorkerContext
we'll need it public...

> > +    // FIXME: nested workers need loading support. Consider adopting ThreadableLoader here.
> Capital "N" is needed in "nested".

Done.

> > -    // FIXME: does this need to provide a charset, like Document::completeURL does?
> > -    return KURL(m_location->url(), url);
> > +
> > +    if (m_encoding.isNull())
> > +        return KURL(m_location->url(), url);
> > +
> > +    return KURL(m_location->url(), url, TextEncoding(m_encoding));
> >  }
> 
> Have you tested that this is what Firefox does? I wouldn't be surprised if it
> always used UTF-8, and didn't inherit the encoding from the document at all.
> 
> This change needs an automated test.

That happened to be a very good suggestion! I've tested FF and realized I'm
adding wrong behavior :-). So I've changed the code and I'm adding the test
that verifies the behavior is same as in FF (and also pretty logical).

> > +        String m_encoding;
> Why is it better to store it as String, not as TextEncoding?

It is only used as a string - to pass into script loader as initial value (now
that we don't need to create TextEncoding in completeURL). So storing String is
now simpler and perhaps faster.

> > +        WorkerThread(const KURL&, const String& userAgent,  const String& encoding, const String& sourceCode, WorkerObjectProxy*);
> Two spaces here.

Done.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.