There are similar code between _computePageRectsWithPrintWidthScaleFactor in WebFrame.mm and WebCore::PrintContext. It might be better that sharing similar code between them before starting to implement new features related with CSS3's PagedMedia. Maybe some code related with printing defined in WebFrame.mm should move to WebCore::PrintContext, and use WebCore::PrintContext from WebKit even in Mac.
Created attachment 50876 [details] refactor-computePageRects
Attachment 50876 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/927020
I've cleared the review flag because the patch needs to be compiled at chromium.
Created attachment 50880 [details] refactor-computePageRects-fix-chromium-build
Comment on attachment 50880 [details] refactor-computePageRects-fix-chromium-build Basically, looks good. r- for a few style nitpicks. > - RenderView* root = toRenderView(m_frame->document()->renderer()); > - > - if (!root) { > - LOG_ERROR("document to be printed has no renderer"); > + if (userScaleFactor <= 0) { > + LOG_ERROR("userScaleFactor has bad value %.2f", userScaleFactor); > return; > } > > + RenderView* root = toRenderView(m_frame->document()->renderer()); > + It seems like NULL check for root was gone. Is this OK? > + const Vector<IntRect>& pageRects = printContext.pageRects(); > + const size_t pageCount = pageRects.size(); > + for (size_t pageNumber = 0; pageNumber < pageCount; ++pageNumber) { > + const IntRect& pageRect = pageRects[pageNumber]; > + NSValue* val = [NSValue valueWithRect: NSMakeRect(pageRect.x(), pageRect.y(), pageRect.width(), pageRect.height())]; > + [pages addObject: val]; - Two more spaces for indent here. - Cannot we use IntRect::operator NSRect() to create NSRect? (maybe it's not in WebCore.base.exp?)
Created attachment 50883 [details] refactor-computePageRects
Thank you for the review. (In reply to comment #5) > (From update of attachment 50880 [details]) > Basically, looks good. r- for a few style nitpicks. > > > - RenderView* root = toRenderView(m_frame->document()->renderer()); > > - > > - if (!root) { > > - LOG_ERROR("document to be printed has no renderer"); > > + if (userScaleFactor <= 0) { > > + LOG_ERROR("userScaleFactor has bad value %.2f", userScaleFactor); > > return; > > } > > > > + RenderView* root = toRenderView(m_frame->document()->renderer()); > > + > > It seems like NULL check for root was gone. Is this OK? The reason I've omitted NULL check for |root| here is that it never happens, I guess. NULL check for m_frame->document()->renderer() is already done in the two callers, computePageRectsWithPageSize and computePageRects. It never happens that |root| becomes NULL in the following code, doesn't it? if (!m_frame->document() || !m_frame->view() || !m_frame->document()->renderer()) return; RenderView* root = toRenderView(m_frame->document()->renderer()); if (!root) { LOG_ERROR("document to be printed has no renderer"); Anyway, for the maximum safety, I've decided to insert NULL check at the head of PrintContext::computePageRectsWithPageSizeInternal in the new patch in case that it is called from other functions in the future. > > > + const Vector<IntRect>& pageRects = printContext.pageRects(); > > + const size_t pageCount = pageRects.size(); > > + for (size_t pageNumber = 0; pageNumber < pageCount; ++pageNumber) { > > + const IntRect& pageRect = pageRects[pageNumber]; > > + NSValue* val = [NSValue valueWithRect: NSMakeRect(pageRect.x(), pageRect.y(), pageRect.width(), pageRect.height())]; > > + [pages addObject: val]; > > - Two more spaces for indent here. > - Cannot we use IntRect::operator NSRect() to create NSRect? (maybe it's not in > WebCore.base.exp?) Done. Thank you! It's already in WebCore.base.exp.
Comment on attachment 50883 [details] refactor-computePageRects This looks sane. I'll land this patch unless anyone object tomorrow. I want to wait a moment in case someone have ideas about this. I asked him to test this manually on Mac by setting scale in the dialog box for printing and looking the print preview. He said it seems working.
Comment on attachment 50883 [details] refactor-computePageRects Clearing flags on attachment: 50883 Committed r56454: <http://trac.webkit.org/changeset/56454>
All reviewed patches have been landed. Closing bug.
ChangeLog fixed in http://trac.webkit.org/changeset/56476.