Bug 102890 – Web Inspector: Viewport console warnings cleanup.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mike West]

Some viewport warnings are errors. Others are logs. I'd suggest that we convert them all to a consistent type, though I don't have a strong opinion on which type makes most sense.

http://marco.org is a good test case; see the attached screenshot for detail.

Handing this to Markus, who's expressed interest in getting started on WebKit work. Hi, Markus! As soon as you've created a bugzilla account, I'll assign the bug to you. :)

[Mike West]

Created attachment 175355 [details]
Screenshot of marco.org.

[Mike West]

(In reply to comment #0)
> Some viewport warnings are errors. Others are logs. I'd suggest that we convert them all to a consistent type, though I don't have a strong opinion on which type makes most sense.

Looking at the code (Source/WebCore/dom/ViewportArguments.cpp:412), the separation is intentional. It makes more sense than I thought. :)

That said, I'd still suggest making the truncation warnings actual warnings, and cleaning up the "Content ignored." message while we're at it for consistency. How about "Viewport argument value "device-width;" for key "width" is invalid, and has been ignored."?

[Markus Heintz]

Created attachment 176458 [details]
Patch

[Markus Heintz]

Created attachment 176459 [details]
Developer console for marco.org after applying the patch to a chromium build

[Markus Heintz]

@Mike: Could you have a look at the patch I uploaded? 

I also tried the patch with a chromium build (pls see the attachment I added). All TruncatedViewportArgumentValueErrors are warnings now.

[Mike West]

I'll take a look shortly. Then we can hand it to a real reviewer. :)

[Mike West]

Comment on attachment 176458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176458&action=review

Thanks Markus, this is looking good! I have a few comments below.

I'd suggest that you clear the r? flag on this patch while you make the changes, and then ask for a real review on the next iteration. :)

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please add a meaningful description of what you've changed here.  Just a sentence or three would be fine.

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

When landing a patch, the commit queue will automagically replace the "Reviewed by" line above. Any other line that contains "OOPS!" will block the patch from landing, so you'll want to get rid of them.

In cases like this, I usually replace this line with something like "No new tests added, as this change is covered by updates to existing test expectations."

> Source/WebCore/ChangeLog:13
> +        (WebCore::viewportErrorMessageTemplate):

It's not _really_ necessary for this patch, but generally you'll want to have a method-by-method description of what exactly you've changed to make things easier for both your reviewer and future-you who has no idea why some change was made.

Here, you might write "Adjusted the console message text for invalid values."

> Source/WebCore/ChangeLog:14
> +        (WebCore::viewportErrorMessageLevel):

And here "Upgraded the truncated viewport  message's MessageLevel to warning."

Again, not particularly useful for _this_ patch, but very useful in general. It's a good habit to get yourself into.

> Source/WebCore/dom/ViewportArguments.cpp:427
>          return TipMessageLevel;

I'd suggest making both of these warnings.

Since you're editing this file already, I'd also suggest putting a FIXME just above the call to addConsoleMessage on line 464, just like the one I've added in http://trac.webkit.org/changeset/135903/trunk/Source/WebCore/html/HTMLFormControlElement.cpp At some point, we'd like to make sure these get moved off the console.

[Markus Heintz]

Created attachment 176666 [details]
Patch

[Markus Heintz]

Comment on attachment 176458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176458&action=review

>> Source/WebCore/ChangeLog:8
>> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> Please add a meaningful description of what you've changed here.  Just a sentence or three would be fine.

OOPS. I didn't really look at the ChangeLog :(. Sry.

Done

>> Source/WebCore/ChangeLog:10
>> +        No new tests (OOPS!).
> 
> When landing a patch, the commit queue will automagically replace the "Reviewed by" line above. Any other line that contains "OOPS!" will block the patch from landing, so you'll want to get rid of them.
> 
> In cases like this, I usually replace this line with something like "No new tests added, as this change is covered by updates to existing test expectations."

Done.

>> Source/WebCore/ChangeLog:13
>> +        (WebCore::viewportErrorMessageTemplate):
> 
> It's not _really_ necessary for this patch, but generally you'll want to have a method-by-method description of what exactly you've changed to make things easier for both your reviewer and future-you who has no idea why some change was made.
> 
> Here, you might write "Adjusted the console message text for invalid values."

Done.

>> Source/WebCore/ChangeLog:14
>> +        (WebCore::viewportErrorMessageLevel):
> 
> And here "Upgraded the truncated viewport  message's MessageLevel to warning."
> 
> Again, not particularly useful for _this_ patch, but very useful in general. It's a good habit to get yourself into.

Done.

>> Source/WebCore/dom/ViewportArguments.cpp:427
>>          return TipMessageLevel;
> 
> I'd suggest making both of these warnings.
> 
> Since you're editing this file already, I'd also suggest putting a FIXME just above the call to addConsoleMessage on line 464, just like the one I've added in http://trac.webkit.org/changeset/135903/trunk/Source/WebCore/html/HTMLFormControlElement.cpp At some point, we'd like to make sure these get moved off the console.

Done.

[Mike West]

Comment on attachment 176666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176666&action=review

Change looks fine to me, with the exception of the ChangeLog.

Once you clean that up, upload a new patch and set r?. Jochen might be interested, or he might prefer that one of the Inspector folks take a look. *shrug*

Thanks!

> Source/WebCore/ChangeLog:-447
> -        

Does your editor perhaps fix whitespace in files automatically? If so, please ask it not to do so for the ChangeLog. If other folks have strangely formatted entries, then so be it. Your changes should simply add to the top, not change the rest of the file.

[Markus Heintz]

Created attachment 176941 [details]
Patch

[Markus Heintz]

Comment on attachment 176666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176666&action=review

>> Source/WebCore/ChangeLog:-447
>> -        
> 
> Does your editor perhaps fix whitespace in files automatically? If so, please ask it not to do so for the ChangeLog. If other folks have strangely formatted entries, then so be it. Your changes should simply add to the top, not change the rest of the file.

Done. :)

Comment on attachment 176941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176941&action=review

Where's the ChangeLog entry for LayoutTests/?

> Source/WebCore/ChangeLog:3
> +       Web Inspector: Viewport console warnings cleanup.

wrong indent

[Markus Heintz]

Created attachment 176947 [details]
Patch

[Markus Heintz]

Comment on attachment 176941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176941&action=review

>> Source/WebCore/ChangeLog:3
>> +       Web Inspector: Viewport console warnings cleanup.
> 
> wrong indent

Done. I also added the modified test files.

[Mike West]

(In reply to comment #15)
> (From update of attachment 176941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176941&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +       Web Inspector: Viewport console warnings cleanup.
> > 
> > wrong indent
> 
> Done. I also added the modified test files.

I think you slightly misunderstood Jochen's comment. When you're back at your desk, I'll show you how to generate a ChangeLog file for the layout tests (WebKit is FULL of ChangeLog files. :) ).

[Markus Heintz]

Created attachment 177445 [details]
Patch

[Markus Heintz]

Thanks Mike and Jochen for showing me how to fix this :).

Hope I fixed everything now :).

[Mike West]

Comment on attachment 177445 [details]
Patch

LGTM, but I don't count. :)

Thanks, Markus.

Comment on attachment 177445 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 177445 [details]
Patch

Rejecting attachment 177445 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
it line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
 + 326db07...4ce7c1f HEAD       -> origin/HEAD  (forced update)
error: Ref refs/remotes/origin/master is at 4ce7c1f16d834fb073b05f260e508f1b149c6499 but expected 326db07dcde36d08554e2c39272b956cbb7020f3
 ! 326db07...4ce7c1f master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15120788

[Mike West]

Comment on attachment 177445 [details]
Patch

Trying again. Looks like the CQ was a bit sick earlier today.

[WebKit Review Bot]

Comment on attachment 177445 [details]
Patch

Clearing flags on attachment: 177445

Committed r136502: <http://trac.webkit.org/changeset/136502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.