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. :)
Created attachment 175355 [details] Screenshot of marco.org.
(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."?
Created attachment 176458 [details] Patch
Created attachment 176459 [details] Developer console for marco.org after applying the patch to a chromium build
@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.
I'll take a look shortly. Then we can hand it to a real reviewer. :)
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.
Created attachment 176666 [details] Patch
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.
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.
Created attachment 176941 [details] Patch
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
Created attachment 176947 [details] Patch
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.
(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. :) ).
Created attachment 177445 [details] Patch
Thanks Mike and Jochen for showing me how to fix this :). Hope I fixed everything now :).
Comment on attachment 177445 [details] Patch LGTM, but I don't count. :) Thanks, Markus.
Comment on attachment 177445 [details] Patch ok
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
Comment on attachment 177445 [details] Patch Trying again. Looks like the CQ was a bit sick earlier today.
Comment on attachment 177445 [details] Patch Clearing flags on attachment: 177445 Committed r136502: <http://trac.webkit.org/changeset/136502>
All reviewed patches have been landed. Closing bug.