Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1489)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 3516010: Initial Print Preview UI. (Closed)

Created:
10 years, 2 months ago by Lei Zhang
Modified:
9 years, 7 months ago
CC:
chromium-reviews, ben+cc_chromium.org
Visibility:
Public.

Description

Initial Print Preview UI. BUG=173 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61425

Patch Set 1 : '' #

Total comments: 1

Patch Set 2 : '' #

Total comments: 1

Patch Set 3 : more renaming #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+352 lines, -4 lines) Patch
M chrome/app/generated_resources.grd View 1 2 1 chunk +5 lines, -0 lines 1 comment Download
M chrome/browser/browser_resources.grd View 1 2 2 chunks +2 lines, -1 line 1 comment Download
M chrome/browser/dom_ui/dom_ui_factory.cc View 1 2 3 chunks +12 lines, -3 lines 0 comments Download
A chrome/browser/dom_ui/print_preview_ui.h View 1 chunk +41 lines, -0 lines 0 comments Download
A chrome/browser/dom_ui/print_preview_ui.cc View 2 1 chunk +87 lines, -0 lines 0 comments Download
A chrome/browser/dom_ui/print_preview_ui_uitest.cc View 1 chunk +48 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview.css View 1 chunk +105 lines, -0 lines 5 comments Download
A chrome/browser/resources/print_preview.html View 1 chunk +49 lines, -0 lines 4 comments Download
M chrome/chrome_browser.gypi View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Lei Zhang
Please add other reviewers as required.
10 years, 2 months ago (2010-10-01 08:28:32 UTC) #1
James Hawkins
You'll need to add the .png in a separate CL. Where did you get the ...
10 years, 2 months ago (2010-10-01 18:25:16 UTC) #2
Lei Zhang
On 2010/10/01 18:25:16, James Hawkins wrote: > You'll need to add the .png in a ...
10 years, 2 months ago (2010-10-01 21:24:31 UTC) #3
James Hawkins
LGTM, but please rename print.[html,css] to print_preview.[html,css]. http://codereview.chromium.org/3516010/diff/14001/15001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/3516010/diff/14001/15001#newcode5012 chrome/app/generated_resources.grd:5012: <message name="IDS_PRINTPREVIEW_TITLE" ...
10 years, 2 months ago (2010-10-04 21:19:09 UTC) #4
Lei Zhang
On 2010/10/04 21:19:09, James Hawkins wrote: > LGTM, but please rename print.[html,css] to print_preview.[html,css]. > ...
10 years, 2 months ago (2010-10-04 22:29:22 UTC) #5
arv (Not doing code reviews)
LGTM Although I would like to know more about how you are planning to actually ...
10 years, 2 months ago (2010-10-06 21:53:10 UTC) #6
Lei Zhang
10 years, 2 months ago (2010-10-06 21:56:46 UTC) #7 
On 2010/10/06 21:53:10, arv wrote:
> LGTM
> 
> Although I would like to know more about how you are planning to actually
> implement this ;-)

Writing a design doc as we speak. See also bugs with label:Feature-PrintPreview.

I wish you got to this code review a bit earlier. I'll send you a separate CL to
address all the nits. I'm a CSS noob so please forgive all the redundant code
and suckage.

> http://codereview.chromium.org/3516010/diff/22001/23001
> File chrome/app/generated_resources.grd (right):
> 
> http://codereview.chromium.org/3516010/diff/22001/23001#newcode5018
> chrome/app/generated_resources.grd:5018: <message
name="IDS_PRINTPREVIEW_TITLE"
> desc="Title for print preview page ">
> IDS_PRINT_PREVIEW_TITLE
> 
> http://codereview.chromium.org/3516010/diff/22001/23002
> File chrome/browser/browser_resources.grd (right):
> 
> http://codereview.chromium.org/3516010/diff/22001/23002#newcode53
> chrome/browser/browser_resources.grd:53: <include name="IDR_PRINTPREVIEW_HTML"
> file="resources\print_preview.html" flattenhtml="true" type="BINDATA" />
> IDS_PRINT_PREVIEW_HTML
> 
> http://codereview.chromium.org/3516010/diff/22001/23007
> File chrome/browser/resources/print_preview.css (right):
> 
> http://codereview.chromium.org/3516010/diff/22001/23007#newcode52
> chrome/browser/resources/print_preview.css:52: margin: 0;
> margin is 0 by default
> 
> http://codereview.chromium.org/3516010/diff/22001/23007#newcode57
> chrome/browser/resources/print_preview.css:57: bottom: 0;
> you can remove left, top, right and bottom here since you are using position:
> relative.
> 
> http://codereview.chromium.org/3516010/diff/22001/23007#newcode72
> chrome/browser/resources/print_preview.css:72: margin: 0;
> same here
> 
> http://codereview.chromium.org/3516010/diff/22001/23007#newcode91
> chrome/browser/resources/print_preview.css:91: margin: 0px;
> s/0px/0/
> 
> http://codereview.chromium.org/3516010/diff/22001/23007#newcode99
> chrome/browser/resources/print_preview.css:99: text-decoration: underline;
> also. padding: 0
> 
> http://codereview.chromium.org/3516010/diff/22001/23008
> File chrome/browser/resources/print_preview.html (right):
> 
> http://codereview.chromium.org/3516010/diff/22001/23008#newcode2
> chrome/browser/resources/print_preview.html:2: <html
> i18n-values="dir:textdirection;" id="t">
> You can remove the id here
> 
> http://codereview.chromium.org/3516010/diff/22001/23008#newcode15
> chrome/browser/resources/print_preview.html:15: <div id="main-content">
> Any reason why you need this div?
> 
> http://codereview.chromium.org/3516010/diff/22001/23008#newcode18
> chrome/browser/resources/print_preview.html:18: <div id="destination-title">
> Looks like  a<hx> tag would be more suitable
> 
> http://codereview.chromium.org/3516010/diff/22001/23008#newcode34
> chrome/browser/resources/print_preview.html:34: <div id="mainview-content">
> I think you can remove all the *-content divs.

Powered by Google App Engine
This is Rietveld 408576698