Improve warnings for non-standard meta tag separators

Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag.

BUG=139428
Committed: https://crrev.com/1e40fdb14a59c824556eef609695fa14738c23a1
Cr-Commit-Position: refs/heads/master@{#419165}

[chaopeng, 2016-09-06 18:21:13]

Description was changed from

==========
Improve warnings for non-standard meta tag separators

BUG=139428
==========

to

==========
Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag. 

BUG=139428
==========

[chaopeng, 2016-09-06 18:21:13]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2016-09-07 15:13:06]

https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp (right):

https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp:155: float
HTMLMetaElement::parsePositiveNumber(Document* document, bool&
hasInvalidSeparator, const String& keyString, const String& valueString, bool*
ok)
In this method and others, lets call the parameter `reportWarnings`, rather than
`hasInvalidSeparator`, and make its meaning opposite.

The variable name in the above method is fine though, that is, from
parseContentAttribute we keep track of hasInvalidSeparator and call
processViewportKeyValuePair which takes `reportWarnings` for which we pass
!hasInvalidSeparator.

https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp:391: void
HTMLMetaElement::reportViewportWarning(Document* document, bool&
hasInvalidSeparator, ViewportErrorCode errorCode, const String& replacement1,
const String& replacement2)
This method shouldn't have this param. Instead, the caller should avoid calling
it based on `reportWarnings`

https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMetaElement.h (right):

https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMetaElement.h:59: static void
processViewportKeyValuePair(Document*, bool& hasInvalidSeparator, const String&
key, const String& value, bool viewportMetaZeroValuesQuirk, void* data);
No need to pass any of these by reference. Just `bool hasInvalidSeperator` is
fine.

[chaopeng, 2016-09-08 19:19:43]

On 2016/09/07 15:13:06, bokan wrote:
>
https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp (right):
> 
>
https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp:155: float
> HTMLMetaElement::parsePositiveNumber(Document* document, bool&
> hasInvalidSeparator, const String& keyString, const String& valueString, bool*
> ok)
> In this method and others, lets call the parameter `reportWarnings`, rather
than
> `hasInvalidSeparator`, and make its meaning opposite.
> 
> The variable name in the above method is fine though, that is, from
> parseContentAttribute we keep track of hasInvalidSeparator and call
> processViewportKeyValuePair which takes `reportWarnings` for which we pass
> !hasInvalidSeparator.
> 
>
https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp:391: void
> HTMLMetaElement::reportViewportWarning(Document* document, bool&
> hasInvalidSeparator, ViewportErrorCode errorCode, const String& replacement1,
> const String& replacement2)
> This method shouldn't have this param. Instead, the caller should avoid
calling
> it based on `reportWarnings`
> 
>
https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/HTMLMetaElement.h (right):
> 
>
https://codereview.chromium.org/2312303002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLMetaElement.h:59: static void
> processViewportKeyValuePair(Document*, bool& hasInvalidSeparator, const
String&
> key, const String& value, bool viewportMetaZeroValuesQuirk, void* data);
> No need to pass any of these by reference. Just `bool hasInvalidSeperator` is
> fine.

PTAL, THANKS

[bokan, 2016-09-08 23:54:57]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-09-08 23:54:57]

lgtm

lgtm, thanks!

[commit-bot: I haz the power, 2016-09-08 23:55:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:00:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 00:00:13]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blink-reviews, 2016-09-09 00:07:12]

Hi David,

It looks something failed. But how to understand the log?

Thanks
Chao

commit-bot@chromium.org via codereview.chromium.org <
reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：

> Try jobs failed on following builders:
> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...
> )
> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...
> )
> chromeos_x86-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
> )
> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> https://codereview.chromium.org/2312303002/
>
-- 
Thank you,
Chao

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-09 00:07:13]

Hi David,

It looks something failed. But how to understand the log?

Thanks
Chao

commit-bot@chromium.org via codereview.chromium.org <
reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：

> Try jobs failed on following builders:
> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...
> )
> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...
> )
> chromeos_x86-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
> )
> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> https://codereview.chromium.org/2312303002/
>
-- 
Thank you,
Chao

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-09-09 00:09:57]

You can click on the red builders in the code review to see what step is
failing on each builder, and click through to the logs. In this case.... :/
I have no idea what's up. It looks unrelated. build.chromium.org shows that
the tree is closed and build is broken so it's unrelated to your change.
I'd wait until later/tomorrow and just try again (you can check the
"Commit" box to try again)

On Thu, Sep 8, 2016 at 8:07 PM, Jianpeng Chao <chaopeng@google.com> wrote:

> Hi David,
>
> It looks something failed. But how to understand the log?
>
> Thanks
> Chao
>
> commit-bot@chromium.org via codereview.chromium.org <
> reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：
>
>> Try jobs failed on following builders:
>> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/cast_shell_linux/builds/221651)
>> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.
>> linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/chromeos_daisy_chromium_compile_only_ng/builds/197319)
>> chromeos_x86-generic_chromium_compile_only_ng on
>> master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/chromeos_x86-generic_chromium_compile_only_ng/builds/197173)
>> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_chromeos_compile_dbg_ng/builds/260322)
>> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_chromeos_rel_ng/builds/275405)
>> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_compile_dbg_ng/builds/154336)
>> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_rel_ng/builds/294722)
>>
>> https://codereview.chromium.org/2312303002/
>>
> --
> Thank you,
> Chao
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-09 00:09:58]

You can click on the red builders in the code review to see what step is
failing on each builder, and click through to the logs. In this case.... :/
I have no idea what's up. It looks unrelated. build.chromium.org shows that
the tree is closed and build is broken so it's unrelated to your change.
I'd wait until later/tomorrow and just try again (you can check the
"Commit" box to try again)

On Thu, Sep 8, 2016 at 8:07 PM, Jianpeng Chao <chaopeng@google.com> wrote:

> Hi David,
>
> It looks something failed. But how to understand the log?
>
> Thanks
> Chao
>
> commit-bot@chromium.org via codereview.chromium.org <
> reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：
>
>> Try jobs failed on following builders:
>> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/cast_shell_linux/builds/221651)
>> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.
>> linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/chromeos_daisy_chromium_compile_only_ng/builds/197319)
>> chromeos_x86-generic_chromium_compile_only_ng on
>> master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/chromeos_x86-generic_chromium_compile_only_ng/builds/197173)
>> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_chromeos_compile_dbg_ng/builds/260322)
>> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_chromeos_rel_ng/builds/275405)
>> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_compile_dbg_ng/builds/154336)
>> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_rel_ng/builds/294722)
>>
>> https://codereview.chromium.org/2312303002/
>>
> --
> Thank you,
> Chao
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-09-09 00:11:05]

Thanks!

David Bokan <bokan@google.com>于2016年9月8日周四 20:09写道：

> You can click on the red builders in the code review to see what step is
> failing on each builder, and click through to the logs. In this case.... :/
> I have no idea what's up. It looks unrelated. build.chromium.org shows
> that the tree is closed and build is broken so it's unrelated to your
> change. I'd wait until later/tomorrow and just try again (you can check the
> "Commit" box to try again)
>
> On Thu, Sep 8, 2016 at 8:07 PM, Jianpeng Chao <chaopeng@google.com> wrote:
>
> Hi David,
>
> It looks something failed. But how to understand the log?
>
> Thanks
> Chao
>
> commit-bot@chromium.org via codereview.chromium.org <
> reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：
>
> Try jobs failed on following builders:
> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...
> )
> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...
> )
> chromeos_x86-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
> )
> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> https://codereview.chromium.org/2312303002/
>
> --
> Thank you,
> Chao
>
>
> --
Thank you,
Chao

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-09 00:11:05]

Thanks!

David Bokan <bokan@google.com>于2016年9月8日周四 20:09写道：

> You can click on the red builders in the code review to see what step is
> failing on each builder, and click through to the logs. In this case.... :/
> I have no idea what's up. It looks unrelated. build.chromium.org shows
> that the tree is closed and build is broken so it's unrelated to your
> change. I'd wait until later/tomorrow and just try again (you can check the
> "Commit" box to try again)
>
> On Thu, Sep 8, 2016 at 8:07 PM, Jianpeng Chao <chaopeng@google.com> wrote:
>
> Hi David,
>
> It looks something failed. But how to understand the log?
>
> Thanks
> Chao
>
> commit-bot@chromium.org via codereview.chromium.org <
> reply@chromiumcodereview-hr.appspotmail.com>于2016年9月8日周四 20:00写道：
>
> Try jobs failed on following builders:
> cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...
> )
> chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...
> )
> chromeos_x86-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
> )
> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> https://codereview.chromium.org/2312303002/
>
> --
> Thank you,
> Chao
>
>
> --
Thank you,
Chao

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bokan, 2016-09-15 13:21:09]

Can we land this?

[chaopeng, 2016-09-16 13:30:43]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2016-09-16 13:31:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 14:38:24]

Description was changed from

==========
Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag. 

BUG=139428
==========

to

==========
Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag. 

BUG=139428
==========

[commit-bot: I haz the power, 2016-09-16 14:38:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-16 14:40:13]

Description was changed from

==========
Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag. 

BUG=139428
==========

to

==========
Improve warnings for non-standard meta tag separators

Ignore other warnings when detected semi-colons in viewport tag.

BUG=139428

Committed: https://crrev.com/1e40fdb14a59c824556eef609695fa14738c23a1
Cr-Commit-Position: refs/heads/master@{#419165}
==========

[commit-bot: I haz the power, 2016-09-16 14:40:14]

Patchset 2 (id:??) landed as
https://crrev.com/1e40fdb14a59c824556eef609695fa14738c23a1
Cr-Commit-Position: refs/heads/master@{#419165}