Improve Document exception messages.

Improve Document exception messages.

BUG=270033
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164574

[Mike West, 2013-12-20 10:54:35]

Sigbjørn, Jochen, would you mind taking a look?

-mike

[sof, 2013-12-20 11:10:53]

Looks fine really.

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:702:
exceptionState.throwDOMException(InvalidCharacterError, "'" + localName + "' is
not a valid element name.");
Tangential, but are there missing checks of exceptionState for various
createElement() calls, here and below? i.e., the uses of 'element' doesn't look
safe.

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3960: StringBuilder message;
String::format() a shorter alternative?

[Mike West, 2013-12-20 11:38:48]

Thanks for taking a look!

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:702:
exceptionState.throwDOMException(InvalidCharacterError, "'" + localName + "' is
not a valid element name.");
On 2013/12/20 11:10:53, sof wrote:
> Tangential, but are there missing checks of exceptionState for various
> createElement() calls, here and below? i.e., the uses of 'element' doesn't
look
> safe.

Good eye. Would you like to take care of that in a separate patch? If not,
please file a bug and I'll try to remember to take care of it. :)

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3960: StringBuilder message;
On 2013/12/20 11:10:53, sof wrote:
> String::format() a shorter alternative?

StringBuilder has an explicit 'append(UChar32)' method that handles random
encoding weirdness. I don't think there's an equiv. on String itself. But maybe
String::format does? I'll take a look.

[Mike West, 2013-12-20 12:00:29]

> StringBuilder has an explicit 'append(UChar32)' method that handles random
> encoding weirdness. I don't think there's an equiv. on String itself. But
maybe
> String::format does? I'll take a look.

Assuming that StringBuilder is doing the right thing (I'm not an encodings guy),
String::format doesn't look like it does the right thing.

[sof, 2013-12-20 12:21:01]

On 2013/12/20 12:00:29, Mike West wrote:
> > StringBuilder has an explicit 'append(UChar32)' method that handles random
> > encoding weirdness. I don't think there's an equiv. on String itself. But
> maybe
> > String::format does? I'll take a look.
> 
> Assuming that StringBuilder is doing the right thing (I'm not an encodings
guy),
> String::format doesn't look like it does the right thing.

Yes, it can't be used.

[sof, 2013-12-20 12:21:11]

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/98543011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:702:
exceptionState.throwDOMException(InvalidCharacterError, "'" + localName + "' is
not a valid element name.");
On 2013/12/20 11:38:49, Mike West wrote:
> On 2013/12/20 11:10:53, sof wrote:
> > Tangential, but are there missing checks of exceptionState for various
> > createElement() calls, here and below? i.e., the uses of 'element' doesn't
> look
> > safe.
> 
> Good eye. Would you like to take care of that in a separate patch? If not,
> please file a bug and I'll try to remember to take care of it. :)

I can take care of it right away.

[Mike West, 2013-12-20 14:07:29]

Ok, latest patch looks good on the bots. Jochen, WDYT?

> > Good eye. Would you like to take care of that in a separate patch? If not,
> > please file a bug and I'll try to remember to take care of it. :)
> 
> I can take care of it right away.

Great! Send me a CL! :)

[jochen (gone - plz use gerrit), 2013-12-20 14:09:48]

lgtm

[commit-bot: I haz the power, 2013-12-20 14:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/98543011/20001

[commit-bot: I haz the power, 2013-12-20 16:50:49]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-07 11:29:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/98543011/150001

[commit-bot: I haz the power, 2014-01-07 13:01:50]

Change committed as 164574