Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
375253002:
[Chrome] Support NumCopies print preset (Closed)
|
Created:
6 years, 5 months ago by Nikhil Modified:
6 years, 1 month ago Reviewers:
Lei Zhang, dmichael (off chromium), yzshen1, lazyboy, Tom Sepez, raymes, Vitaly Buka (NO REVIEWS) CC:
yzshen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSupport NumCopies print preset
Chrome side changes to support NumCopies print preset option.
Copies in print preview dialog should be set to value set for
NumCopies in pdf source document.
BUG=169120
Committed: https://crrev.com/64c4daa7d64fc28efd1e49bf3548323eb67a32ec
Cr-Commit-Position: refs/heads/master@{#304785}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Rebase #Patch Set 3 : Review feedback (update method name) #
Total comments: 3
Patch Set 4 : Review feedback (add version tag) #Patch Set 5 : PPB_Printing_Dev changes #
Total comments: 11
Patch Set 6 : Rebase #Patch Set 7 : PPP_Pdf changes #
Total comments: 12
Patch Set 8 : Review feedback (oop proxy, out-param) #
Total comments: 21
Patch Set 9 : Review feedback (move struct to PPP_Pdf) #
Total comments: 2
Patch Set 10 : Review feedback (rename from PPP_.. to PP_..) #Patch Set 11 : Rebase #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : Use PP_ToBool #Patch Set 15 : Rebase #
Messages
Total messages: 90 (12 generated)
This is a _WIP_ patch for Chrome side change. But please do take a look to check whether current approach looks fine or way off. * I followed implementation similar to PrintScaling. I'm sending an IPC message from PrintWebViewHelper::OnPrintPreview() upon checking NumCopies print property of pdf file. * Above IPC message should probably be handled in PrintPreviewUI::OnPrintPreviewNumCopies() to call javascript function to update value. Please let me know if you think some other approach should be followed. Thanks! https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:610: // FIXME: Call javascript function? Is this right approach? https://codereview.chromium.org/375253002/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1042: Send(new PrintHostMsg_PrintPreviewNumCopies(routing_id(), num_copies)); Is this right approach?
thanks, looks like a correct approach https://codereview.chromium.org/375253002/diff/1/chrome/browser/printing/prin... File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/printing/prin... chrome/browser/printing/print_preview_message_handler.cc:204: void PrintPreviewMessageHandler::OnPrintPreviewNumCopies(int num_copies) { PrintPreviewNumCopies -> PrintPreviewSetCopies https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:610: // FIXME: Call javascript function? yes On 2014/07/09 09:36:47, Nikhil wrote: > Is this right approach? https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview/print_preview_ui.h:147: void OnPrintPreviewNumCopies(int num_copies); *SetCopies https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages.h File chrome/common/print_messages.h (right): https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages... chrome/common/print_messages.h:448: IPC_MESSAGE_ROUTED0(PrintHostMsg_PrintPreviewScalingDisabled) Let's create structure with all options we'd like to get from PDF, including PrintPreviewScalingDisabled and send it as single message struct PrintHostMsg_SetOptionsFromDocument { } https://codereview.chromium.org/375253002/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1037: // Send a message to browser if number of copies to be printed information is Should be inside of: if (print_pages_params_->params.is_first_request && !print_preview_context_.IsModifiable() && https://codereview.chromium.org/375253002/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1042: Send(new PrintHostMsg_PrintPreviewNumCopies(routing_id(), num_copies)); On 2014/07/09 09:36:47, Nikhil wrote: > Is this right approach? yes https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_plugin_instance_impl.h:242: int GetNumCopies(); GetCopiesToPrint() https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_webplugin_impl.h (right): https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_webplugin_impl.h:79: virtual int getNumCopies() OVERRIDE; getCopiesToPrint https://codereview.chromium.org/375253002/diff/1/pdf/instance.h File pdf/instance.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/instance.h#newcode90 pdf/instance.h:90: virtual int32_t GetNumCopies() OVERRIDE; Name https://codereview.chromium.org/375253002/diff/1/pdf/out_of_process_instance.h File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.h:76: virtual int32_t GetNumCopies() OVERRIDE; name it's just a function of the plugin interface, name should point that it's printing copies https://codereview.chromium.org/375253002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/pdf_engine.h#newcode251 pdf/pdf_engine.h:251: virtual int GetNumCopies() = 0; name https://codereview.chromium.org/375253002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/pdfium/pdfium_engine.h#n... pdf/pdfium/pdfium_engine.h:92: virtual int GetNumCopies(); name https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... ppapi/api/dev/ppp_printing_dev.idl:63: int32_t NumCopies([in] PP_Instance instance); name https://codereview.chromium.org/375253002/diff/1/ppapi/cpp/dev/printing_dev.h File ppapi/cpp/dev/printing_dev.h (right): https://codereview.chromium.org/375253002/diff/1/ppapi/cpp/dev/printing_dev.h... ppapi/cpp/dev/printing_dev.h:33: virtual int32_t GetNumCopies() = 0; GetCopies
+yzshen, who may give us some input on Pepper changes
Glad to see approach looks fine. I'll explore how to update value through javascript function. Please check one query that I've regarding one of the comments. I'll incorporate other review comments in next patch. Thanks! https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages.h File chrome/common/print_messages.h (right): https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages... chrome/common/print_messages.h:448: IPC_MESSAGE_ROUTED0(PrintHostMsg_PrintPreviewScalingDisabled) On 2014/07/10 01:13:05, Vitaly Buka wrote: > Let's create structure with all options we'd like to get from PDF, including > PrintPreviewScalingDisabled and send it as single message > struct PrintHostMsg_SetOptionsFromDocument { > } Shall I create another CL for this first? To introduce new structure and making relevant changes for PrintScaling option. Or you'd prefer to have it as part of this CL?
On 2014/07/10 14:46:53, Nikhil wrote: > Glad to see approach looks fine. I'll explore how to update value through > javascript function. > > Please check one query that I've regarding one of the comments. I'll incorporate > other review comments in next patch. > > Thanks! > > https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages.h > File chrome/common/print_messages.h (right): > > https://codereview.chromium.org/375253002/diff/1/chrome/common/print_messages... > chrome/common/print_messages.h:448: > IPC_MESSAGE_ROUTED0(PrintHostMsg_PrintPreviewScalingDisabled) > On 2014/07/10 01:13:05, Vitaly Buka wrote: > > Let's create structure with all options we'd like to get from PDF, including > > PrintPreviewScalingDisabled and send it as single message > > struct PrintHostMsg_SetOptionsFromDocument { > > } > > Shall I create another CL for this first? To introduce new structure and making > relevant changes for PrintScaling option. Or you'd prefer to have it as part of > this CL? I have no preference here. smaller changes are better, but I guess introducing new fields would touch almost the same code. I guess you can leave JS changes for future CLs.
On 2014/07/10 17:26:20, Vitaly Buka wrote:
> On 2014/07/10 14:46:53, Nikhil wrote:
> > On 2014/07/10 01:13:05, Vitaly Buka wrote:
> > > Let's create structure with all options we'd like to get from PDF,
including
> > > PrintPreviewScalingDisabled and send it as single message
> > > struct PrintHostMsg_SetOptionsFromDocument {
> > > }
> >
> > Shall I create another CL for this first? To introduce new structure and
> making
> > relevant changes for PrintScaling option. Or you'd prefer to have it as part
> of
> > this CL?
>
> I have no preference here. smaller changes are better, but I guess introducing
> new fields would touch almost the same code.
> I guess you can leave JS changes for future CLs.
Then let's try to have smaller changes. Maybe we can do the following if you
agree -
* Introduce new structure, PrintWebViewHelper::SetOptionsFromDocument(), and
PrintPreviewUI::SetOptionsFromDocument(). We call JS functions from
PrintPreviewUI::SetOptionsFromDocument() based on values set in
PrintWebViewHelper::SetOptionsFromDocument().
* Submit pdfium changes (https://codereview.chromium.org/345123002/)
* Submit Blink changes after review (https://codereview.chromium.org/379793002/)
* Update and submit this patch.
* JS changes.
If this sounds okay, then please let me know and I'll get on it.
Thanks!
https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... ppapi/api/dev/ppp_printing_dev.idl:63: int32_t NumCopies([in] PP_Instance instance); Style nits: Usually the name of a method should be VerbNoun(), so maybe GetNumberOfCopies()? Besides, please put it in a newer version (by updating label struct above, and also adding [version=XXX] tag.) You probably also want to notify pepper-dev@chromium.org (or pepper-team@google.com if you want to have internal discussion) about your proposed interface change.
Rebase to latest and on top of https://codereview.chromium.org/388783003/
Review comments incorporated. PTAL, Thanks! https://codereview.chromium.org/375253002/diff/1/chrome/browser/printing/prin... File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/printing/prin... chrome/browser/printing/print_preview_message_handler.cc:204: void PrintPreviewMessageHandler::OnPrintPreviewNumCopies(int num_copies) { On 2014/07/10 01:13:04, Vitaly Buka wrote: > PrintPreviewNumCopies -> PrintPreviewSetCopies Removed, OnSetOptionsFromDocument() will be used. https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/375253002/diff/1/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview/print_preview_ui.h:147: void OnPrintPreviewNumCopies(int num_copies); On 2014/07/10 01:13:05, Vitaly Buka wrote: > *SetCopies Removed, OnSetOptionsFromDocument() will be used. https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_plugin_instance_impl.h:242: int GetNumCopies(); On 2014/07/10 01:13:05, Vitaly Buka wrote: > GetCopiesToPrint() Done. https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_webplugin_impl.h (right): https://codereview.chromium.org/375253002/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_webplugin_impl.h:79: virtual int getNumCopies() OVERRIDE; On 2014/07/10 01:13:05, Vitaly Buka wrote: > getCopiesToPrint Done. https://codereview.chromium.org/375253002/diff/1/pdf/instance.h File pdf/instance.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/instance.h#newcode90 pdf/instance.h:90: virtual int32_t GetNumCopies() OVERRIDE; On 2014/07/10 01:13:05, Vitaly Buka wrote: > Name Done. https://codereview.chromium.org/375253002/diff/1/pdf/out_of_process_instance.h File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.h:76: virtual int32_t GetNumCopies() OVERRIDE; On 2014/07/10 01:13:05, Vitaly Buka wrote: > name > it's just a function of the plugin interface, name should point that it's > printing copies Done. https://codereview.chromium.org/375253002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/pdf_engine.h#newcode251 pdf/pdf_engine.h:251: virtual int GetNumCopies() = 0; On 2014/07/10 01:13:05, Vitaly Buka wrote: > name Done. https://codereview.chromium.org/375253002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/375253002/diff/1/pdf/pdfium/pdfium_engine.h#n... pdf/pdfium/pdfium_engine.h:92: virtual int GetNumCopies(); On 2014/07/10 01:13:05, Vitaly Buka wrote: > name Done. https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... ppapi/api/dev/ppp_printing_dev.idl:63: int32_t NumCopies([in] PP_Instance instance); On 2014/07/10 01:13:05, Vitaly Buka wrote: > name Done. https://codereview.chromium.org/375253002/diff/1/ppapi/api/dev/ppp_printing_d... ppapi/api/dev/ppp_printing_dev.idl:63: int32_t NumCopies([in] PP_Instance instance); On 2014/07/14 17:30:55, yzshen1 wrote: > Style nits: Usually the name of a method should be VerbNoun(), so maybe > GetNumberOfCopies()? > > Besides, please put it in a newer version (by updating label struct above, and > also adding [version=XXX] tag.) > I'm unsure of this change. Please check and let me know if something should be changed. > You probably also want to notify mailto:pepper-dev@chromium.org (or > mailto:pepper-team@google.com if you want to have internal discussion) about your > proposed interface change. https://codereview.chromium.org/375253002/diff/1/ppapi/cpp/dev/printing_dev.h File ppapi/cpp/dev/printing_dev.h (right): https://codereview.chromium.org/375253002/diff/1/ppapi/cpp/dev/printing_dev.h... ppapi/cpp/dev/printing_dev.h:33: virtual int32_t GetNumCopies() = 0; On 2014/07/10 01:13:05, Vitaly Buka wrote: > GetCopies Done.
https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... ppapi/api/dev/ppp_printing_dev.idl:64: int32_t GetCopiesToPrint([in] PP_Instance instance); You need to add [version=0.7] for this method. Besides, please consider sending a mail to the pepper team about your proposed interface change.
https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... ppapi/api/dev/ppp_printing_dev.idl:64: int32_t GetCopiesToPrint([in] PP_Instance instance); On 2014/07/16 16:10:46, yzshen1 wrote: > You need to add [version=0.7] for this method. > > Besides, please consider sending a mail to the pepper team about your proposed > interface change. Thanks for reviewing. When I tried adding [version=0.7] earlier, I was not able to upload the patch. It gave presubmit error and suggested to run the generator (I didn't know what to do for this). I'll email pepper team explaining the requirement and proposed change.
vitalybuka@ - Let's try to complete this one now. Related patches for Blink and pdfium have already landed (https://codereview.chromium.org/379793002/, https://codereview.chromium.org/345123002/). yzshen1@ - Added version tag. PTAL. I'll send mail to pepper-team now regarding interface change. Thanks! https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... File ppapi/api/dev/ppp_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/60001/ppapi/api/dev/ppp_printi... ppapi/api/dev/ppp_printing_dev.idl:64: int32_t GetCopiesToPrint([in] PP_Instance instance); On 2014/07/16 16:10:46, yzshen1 wrote: > You need to add [version=0.7] for this method. > > Besides, please consider sending a mail to the pepper team about your proposed > interface change. Done.
yzshen1@ - I tried sending an email to 'pepper-team@google.com' as you suggested. But it looks like that I don't have permissions to post to this group. I received delivery failed notification.
On 2014/07/29 11:17:08, Nikhil wrote: > yzshen1@ - I tried sending an email to mailto: as you > suggested. But it looks like that I don't have permissions to post to this > group. I received delivery failed notification. Then please try pepper-dev@chromium.org? (Thanks for doing this!)
On 2014/07/29 17:15:34, yzshen1 wrote: > On 2014/07/29 11:17:08, Nikhil wrote: > > yzshen1@ - I tried sending an email to mailto: as you > > suggested. But it looks like that I don't have permissions to post to this > > group. I received delivery failed notification. > > Then please try mailto:pepper-dev@chromium.org? (Thanks for doing this!) Looks like I was able to send to this one. I added both you and Vitaly also. PTAL at changes. Once the changes look good to both you and Vitaly, I'll request for OWNERS reviews. Thanks!
Since we don't have to support backwards compatibility for this API, I don't have a big problem with you adding this feature. It shouldn't add much complexity. However... I don't see any changes to deal with the out-of-process version? +raymes, who has been working on that. We're moving PDF out-of-process. See: https://code.google.com/p/chromium/issues/detail?id=303491 ...so you're going to also have to modify stuff in ppapi/proxy. Also, the way you've designed this function, I think you'll be requiring a renderer->plugin sync message. I'm not sure how far raymes has thought about this, but we should probably clean it up to reduce these sync messages instead of adding new ones. E.g., instead of a PpapiMsg_PPPPRinting_QuerySupportedFormats message, maybe the proxy needs a: PpapiMsg_PPPPrinting_QueryConfiguration ... and the plugin-side of the proxy can call all of: QuerySupportedFormats IsScalingDisabled and GetNumPages ...when it gets that message, and return all the results in 1 message reply. (Maybe ultimately there's an even better way, but the current 2 roundtrips is a little silly, and 3 would be sillier). Thanks for working on this!
On 2014/07/30 16:13:56, dmichael wrote: > Since we don't have to support backwards compatibility for this API, I don't > have a big problem with you adding this feature. It shouldn't add much > complexity. > > However... I don't see any changes to deal with the out-of-process version? > +raymes, who has been working on that. We're moving PDF out-of-process. See: > https://code.google.com/p/chromium/issues/detail?id=303491 > > ...so you're going to also have to modify stuff in ppapi/proxy. > > Also, the way you've designed this function, I think you'll be requiring a > renderer->plugin sync message. I'm not sure how far raymes has thought about > this, but we should probably clean it up to reduce these sync messages instead > of adding new ones. > > E.g., instead of a PpapiMsg_PPPPRinting_QuerySupportedFormats message, maybe the > proxy needs a: > PpapiMsg_PPPPrinting_QueryConfiguration > ... > and the plugin-side of the proxy can call all of: > QuerySupportedFormats > IsScalingDisabled > and > GetNumPages > ...when it gets that message, and return all the results in 1 message reply. > > (Maybe ultimately there's an even better way, but the current 2 roundtrips is a > little silly, and 3 would be sillier). > > Thanks for working on this! Hi, David. I don't remember. Do we need to worry about the scenario that an older version of Chrome (using PPP Printing v0.6) works with a plugin supporting v0.7 (and the other way around)? Or we know that PDF is the only user and the two sides stay in sync? To Nikhil: If we do have to handle multiple versions, then in ppapi/cpp/dev/printing_dev.cc you need to register both versions of the PPP Printing APIs. And at the browser side, it needs to try getting v0.7 and fall back to v0.6 if v0.7 is not available. If we don't need to worry about version mismatch, we could consider deleting the v0.6 annotation.
raymes@ - Could you take a look at this and share your opinion?
On 2014/07/31 04:58:10, Nikhil wrote: > raymes@ - Could you take a look at this and share your opinion? Looks fine for me, thanks for your patch. Since this is only being used by PDF, which is shipped with Chrome I don't think the versioning matters too much. It would matter if we tried to use it in flash I think. Does that make sense Yuzhu or am I missing something also? :) Could you please also test the out of process version with the --out-of-process-pdf flag to Chrome. As David noted, it looks like there isn't an IPC there yet. You might need to change ppapi/proxy/ppp_printing_proxy.cc. I agree with David that it would be nice to merge those IPCs into a single one, but I would be ok if that was done in a separate CL. I wish there was a way to add a test for this type of thing by AFAIK there isn't an easy way for ppp interfaces. Please correct me if I'm wrong, David :)
Thank you all for taking a look at this! A few details from my side - [1] I was hoping to support print preset option features, one at a time, as per PDF spec (Issue 169120, which is this patch is about). pdfium changes have already landed for this. [2] I was also planning to support open parameter features for PDF, one at a time, as per PDF spec (Issue 64309). PTAL at https://codereview.chromium.org/420063002/ for zoom parameter. Unfortunately, I didn't know about out-of-process (OOP) version and as of now I don't fully understand the implementation differences needed to support these features in OOP version. I tried launching Chrome by providing --out-of-process-pdf flag and as expected, current patch didn't work. It will be great if I can be pointed in right direction to develop a better understanding. FWIW, I see only Raymes is making OOP related changes. I'd be motivated to get involved and support the development of OOP and then come back to these features. As it stands, I don't understand the way forward for these features :/
On 2014/07/31 06:59:51, raymes wrote: > On 2014/07/31 04:58:10, Nikhil wrote: > > raymes@ - Could you take a look at this and share your opinion? > > Looks fine for me, thanks for your patch. Since this is only being used by PDF, > which is shipped with Chrome I don't think the versioning matters too much. It > would matter if we tried to use it in flash I think. Does that make sense Yuzhu > or am I missing something also? :) I agree with you. In that case, I think we could remove the v0.6 annotation altogether. (I still think increasing the version number to 0.7 is good.) > > Could you please also test the out of process version with the > --out-of-process-pdf flag to Chrome. As David noted, it looks like there isn't > an IPC there yet. You might need to change ppapi/proxy/ppp_printing_proxy.cc. I > agree with David that it would be nice to merge those IPCs into a single one, > but I would be ok if that was done in a separate CL. > > I wish there was a way to add a test for this type of thing by AFAIK there isn't > an easy way for ppp interfaces. Please correct me if I'm wrong, David :)
On 2014/07/31 12:42:33, Nikhil wrote: > Thank you all for taking a look at this! A few details from my side - > > [1] I was hoping to support print preset option features, one at a time, as per > PDF spec (Issue 169120, which is this patch is about). pdfium changes have > already landed for this. > > [2] I was also planning to support open parameter features for PDF, one at a > time, as per PDF spec (Issue 64309). PTAL at > https://codereview.chromium.org/420063002/ for zoom parameter. > > Unfortunately, I didn't know about out-of-process (OOP) version and as of now I > don't fully understand the implementation differences needed to support these > features in OOP version. I tried launching Chrome by providing > --out-of-process-pdf flag and as expected, current patch didn't work. It will be > great if I can be pointed in right direction to develop a better understanding. > > FWIW, I see only Raymes is making OOP related changes. I'd be motivated to get > involved and support the development of OOP and then come back to these > features. > > As it stands, I don't understand the way forward for these features :/ Hi Nikhil, I think the only thing you will have to do to make this work out of process is to modify the proxy. If you take a look at ppapi/proxy/ppp_printing_proxy.cc you will see that we have to send messages between processes for the function calls in PPP_Printing. We will need the same thing for this function. It would be great to get you involved in OOP PDF if you are interested. I would be happy to meet with you if you are interested in finding out more :) Please mail me directly if you want to set something up. Thanks, Raymes
On 2014/07/31 23:04:42, raymes wrote: > > Hi Nikhil, > > I think the only thing you will have to do to make this work out of process is > to modify the proxy. If you take a look at ppapi/proxy/ppp_printing_proxy.cc > you will see that we have to send messages between processes for the function > calls in PPP_Printing. We will need the same thing for this function. > I tried it, but it didn't work. I fear something is broken at print_web_view_helper.cc layer as I couldn't see any of my added logs. But maybe we should discuss this in a separate CL. > It would be great to get you involved in OOP PDF if you are interested. I would > be happy to meet with you if you are interested in finding out more :) Please > mail me directly if you want to set something up. > This is great! I've sent you an email. Please check :) > Thanks, > Raymes
On 2014/07/31 17:23:00, yzshen1 wrote: > On 2014/07/31 06:59:51, raymes wrote: > > On 2014/07/31 04:58:10, Nikhil wrote: > > > raymes@ - Could you take a look at this and share your opinion? > > > > Looks fine for me, thanks for your patch. Since this is only being used by > PDF, > > which is shipped with Chrome I don't think the versioning matters too much. It > > would matter if we tried to use it in flash I think. Does that make sense > Yuzhu > > or am I missing something also? :) > I agree with you. > In that case, I think we could remove the v0.6 annotation altogether. (I still > think increasing the version number to 0.7 is good.) > > yzshen1@ - If this is the way forward, then please leave a review comment. I'll update the patch accordingly :) > > Could you please also test the out of process version with the > > --out-of-process-pdf flag to Chrome. As David noted, it looks like there isn't > > an IPC there yet. You might need to change ppapi/proxy/ppp_printing_proxy.cc. > I > > agree with David that it would be nice to merge those IPCs into a single one, > > but I would be ok if that was done in a separate CL. > > > > I wish there was a way to add a test for this type of thing by AFAIK there > isn't > > an easy way for ppp interfaces. Please correct me if I'm wrong, David :)
On 2014/08/04 11:48:28, Nikhil wrote: > On 2014/07/31 17:23:00, yzshen1 wrote: > > On 2014/07/31 06:59:51, raymes wrote: > > > On 2014/07/31 04:58:10, Nikhil wrote: > > > > raymes@ - Could you take a look at this and share your opinion? > > > > > > Looks fine for me, thanks for your patch. Since this is only being used by > > PDF, > > > which is shipped with Chrome I don't think the versioning matters too much. > It > > > would matter if we tried to use it in flash I think. Does that make sense > > Yuzhu > > > or am I missing something also? :) > > I agree with you. > > In that case, I think we could remove the v0.6 annotation altogether. (I still > > think increasing the version number to 0.7 is good.) > > > > > > yzshen1@ - If this is the way forward, then please leave a review comment. I'll > update the patch accordingly :) yzshen is on another team now, so he may not be responsive to Pepper stuff. Yes, just remove the 0.6 version annotation (and don't worry about backwards compatibility).
Looking at this again, I don't know what I was thinking with my prior comment. I'm quite sure that we DO need to worry about backward compatibility for flash which I believe /does/ use this interface. I guess I was thinking that flash will never use the new function you are adding, so we don't need to worry about that case, but I think we still need to do what Yuzhu was suggesting. We will need to keep the version annotation. I'm really sorry for the confusion :(
On 2014/08/05 06:25:31, raymes wrote: > Looking at this again, I don't know what I was thinking with my prior comment. > I'm quite sure that we DO need to worry about backward compatibility for flash > which I believe /does/ use this interface. > > I guess I was thinking that flash will never use the new function you are > adding, so we don't need to worry about that case, but I think we still need to > do what Yuzhu was suggesting. We will need to keep the version annotation. > > I'm really sorry for the confusion :( Hmm, I just looked at the usage numbers for PPB_Printing_Dev, and that agrees with what Raymes is saying. I don't know why it doesn't show up in Flash's manifest file, but it does look like Flash is using it. Sorry for the confusion. This patch is still missing: 1) out-of-process and 2) backwards compatibility ...which will both make it significantly more complex, I'm leaning towards saying this feature isn't worth the complexity it will bring.
On 2014/08/05 15:01:38, dmichael wrote: > On 2014/08/05 06:25:31, raymes wrote: > > Looking at this again, I don't know what I was thinking with my prior comment. > > I'm quite sure that we DO need to worry about backward compatibility for flash > > which I believe /does/ use this interface. > > > > I guess I was thinking that flash will never use the new function you are > > adding, so we don't need to worry about that case, but I think we still need > to > > do what Yuzhu was suggesting. We will need to keep the version annotation. > > > > I'm really sorry for the confusion :( > Hmm, I just looked at the usage numbers for PPB_Printing_Dev, and that agrees > with what Raymes is saying. I don't know why it doesn't show up in Flash's > manifest file, but it does look like Flash is using it. Sorry for the confusion. > > This patch is still missing: > 1) out-of-process > and > 2) backwards compatibility > > ...which will both make it significantly more complex, I'm leaning towards > saying this feature isn't worth the complexity it will bring. Sorry for late reply. The manifest file only records those "must" interfaces. If Flash can work reasonably without an API, it doesn't have to put the API in the manifest file. Besides, I think sometimes people forgot to update the file.
Thank you all for sharing your views on this! I'm still unsure regarding the way forward for this feature. Is there no way through which we can support this? Not even after OOP release? As per the PDF spec, PrintScaling, Duplex, NumCopies and PrintPageRange are print preset features that can be supported. PrintScaling is already supported in Chrome, and it only makes sense to me to support remaining ones as well. Please let me know WDYT. Thanks!
On 2014/08/08 09:55:39, Nikhil wrote: > Thank you all for sharing your views on this! I'm still unsure regarding the way > forward for this feature. > > Is there no way through which we can support this? Not even after OOP release? > > As per the PDF spec, PrintScaling, Duplex, NumCopies and PrintPageRange are > print preset features that can be supported. PrintScaling is already supported > in Chrome, and it only makes sense to me to support remaining ones as well. > > Please let me know WDYT. > > Thanks! Hmm... Pepper team is just kind of small right now and we get a lot of API requests. I think yours is a good one, and *relatively* low risk, because we're only talking about exposing it to PDF (and maybe Flash if Adobe chooses to use it). If you're willing to also share the burden of taking and fixing bugs that come up in the new code, and also helping make sure that out-of-process PDF doesn't regress, I think it's fine. Raymes: You're probably the best reviewer for this; do you have time and are you willing to handle the reviews for the feature? To recap, I think the key things to fix before we come back with another review are: 1) Make it work out-of-process. 2) Make it backwards compatible. While doing 1), please try to reduce sync IPC messages instead of adding more (see my previous comment). Thanks again for working on it.
On 2014/08/08 15:48:04, dmichael wrote: > On 2014/08/08 09:55:39, Nikhil wrote: > > Thank you all for sharing your views on this! I'm still unsure regarding the > way > > forward for this feature. > > > > Is there no way through which we can support this? Not even after OOP release? > > > > As per the PDF spec, PrintScaling, Duplex, NumCopies and PrintPageRange are > > print preset features that can be supported. PrintScaling is already supported > > in Chrome, and it only makes sense to me to support remaining ones as well. > > > > Please let me know WDYT. > > > > Thanks! > Hmm... Pepper team is just kind of small right now and we get a lot of API > requests. I think yours is a good one, and *relatively* low risk, because we're > only talking about exposing it to PDF (and maybe Flash if Adobe chooses to use > it). > > If you're willing to also share the burden of taking and fixing bugs that come > up in the new code, and also helping make sure that out-of-process PDF doesn't > regress, I think it's fine. > > Raymes: You're probably the best reviewer for this; do you have time and are you > willing to handle the reviews for the feature? > > To recap, I think the key things to fix before we come back with another review > are: > 1) Make it work out-of-process. > 2) Make it backwards compatible. > While doing 1), please try to reduce sync IPC messages instead of adding more > (see my previous comment). > > Thanks again for working on it. Oh, and let me try to offer you some hints for backwards-compatibility... we don't have a lot of examples of that for PPP interfaces right now. You'll probably want some kind of wrapper class that you initialize with whichever version of PPP_Printing is supported by the plugin, kind of like PPP_Instance_Combined: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... Typically for PPP interfaces, the renderer side of the proxy only speaks 1 version of the API, and the plugin side of the proxy is responsible for "translating" to whatever the plugin really supports. For example, in this case, you might have the renderer speaking version "0.7". The plugin side of the proxy will know that the plugin (for the case of Flash) is actually using version "0.6", so when it gets a request for "NumCopies" from the renderer, instead of asking the plugin (which has no such function to call), it simply responds with 1 as if it had called the plugin's NumCopies and it returned 1. Also, it's important to note that *each time* you modify the API, you should maintain backwards compatibility for *every* prior version. This API being Private, if we're super careful, we could possibly avoid that if we know Flash stays on 0.6. But ideally, each version of the API we make available should be supported forevermore. So... you might consider saving up all your API changes for 1 revision, to keep the backwards compatibility code as simple as possible.
As an alternative to changing the PPP_Printing interface, can we just change the PPB_PDF interface and add a SetNumCopies? That would be much simpler - we wouldn't have to worry about backward compatibility and we wouldn't have to worry about PPP interfaces which aren't very nice :(
Hey David, just looping back on this for Nikhil. How do you feel about adding a new function to PPB_Printing_Dev which is SetDefaultNumCopies? Changing this would be much easier than changing the PPP interface and might make this change feasible. I would be ok with doing that.
On 2014/08/22 05:16:24, raymes wrote: > Hey David, just looping back on this for Nikhil. How do you feel about adding a > new function to PPB_Printing_Dev which is SetDefaultNumCopies? Changing this > would be much easier than changing the PPP interface and might make this change > feasible. I would be ok with doing that. Thanks for pinging me; I meant to loop back but forgot after my vacation. I think that sounds like a great idea. It's not exactly pretty, since it will require yet another sync IPC (plugin->renderer). If it goes on PPB_Printing_Dev, it still has to have backwards compatibility for Flash. But the code for that would be much, much easier than doing backwards compat in PPP_Printing_Dev. (PPB_Pdf like your previous message would also be OK with me, since it would avoid backwards-compat entirely. But PPB_Printing_Dev shouldn't be hard to do and might be simpler to wire up internally)
Thanks raymes@ and dmichael@ for discussing this! Based on your suggestion, I'll use PPB_Printing_Dev interface :) Once I figure out how to get information from pdfium_engine via PPB_Printing_Dev (any hints are welcomed!), I'll try to minimize number of IPC messages required and will try to save up all API changes for one revision.
dmichael@ - Could you please point me in right direction to develop understanding to modify PPB_Printing_Dev interface? I can't seem to find a good starting point to understand the code flow. Thanks.
On 2014/08/27 14:47:40, Nikhil wrote: > dmichael@ - Could you please point me in right direction to develop > understanding to modify PPB_Printing_Dev interface? I can't seem to find a good > starting point to understand the code flow. Thanks. Modify ppb_printing_dev.idl to add a new version (0.8) and a new function at the end; it's very similar to how you've done the PPP interface. PPP interfaces are functions the browser calls on the plugin, and PPB interfaces are functions the plugin calls on the browser. So in this case, you will probably make a "SetNumCopies" function instead of a "GetNumCopies". You'll edit the PPB_Printing_API to add the new function: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/thunk/ppb_pr... And you'll edit ppb_printing_dev_thunk to add a new struct that is of type PPB_Printing_Dev_0_8 (and you'll leave the PPB_Printing_Dev_0_8 there). You'll also want a "GetPPB_Printing_Dev_0_8_Thunk()" function in addition to the existing one for 0.7. You'll add a line for the 0.8 version here: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/thunk/interf... ^^^^^ That all should get you a backwards-compatible interface. That's kind of the easy part. What's not clear to me, and maybe you and Raymes can figure out, is what's the interaction model of this. Here's one guess at how it might work: * SetNumCopies must be called some time after creating the PPB_Printing_Dev resource but before returning from PPP_Printing_Dev::PrintPages. * When SetNumCopies is called, PrintingResource sets a local variable num_pages_ (that defaulted to 1). * The sync message PpapiMsg_PPPPrinting_PrintPages gets a new output field for the number of pages... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_... * Somehow the PPP_Printing_Dev looks up the number of pages and puts it in the reply... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppp_pr... I'm not quite sure how this last step happens. Maybe num_pages has to be sort of static... blech... Maybe you guys can think of something better.
vitalybuka@ - Could you please take a look at this? I've made PPB_Printing_Dev related changes. But, it seems that earlier approach to use PrintHostMsg_SetOptionsFromDocument IPC to inform browser to update print preview doesn't seem to work now. Is there any other way to inform browser to update print preview settings? Thanks!
https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:102: case PpapiHostMsg_PrintHost_Create::ID: { this should be "case" in switch on line 91 https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_printing_renderer_host.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_printing_renderer_host.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Copyright (c) 2014 -> Copyright 2014 please update other files of your change to https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_printing_renderer_host.cc:56: PrintWebViewHelper* print_view_helper = PrintWebViewHelper::Get(render_view); why not to send message to browser directly, without using helper? https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1015: PrintHostMsg_SetOptionsFromDocument_Params params; Why this one is not enough? becase of out-of-process? So scaling also didn't work before? https://codereview.chromium.org/375253002/diff/100001/ppapi/api/dev/ppb_print... File ppapi/api/dev/ppb_printing_dev.idl (right): https://codereview.chromium.org/375253002/diff/100001/ppapi/api/dev/ppb_print... ppapi/api/dev/ppb_printing_dev.idl:57: [in] PP_PrintPresetOptions_Dev print_options); long line https://codereview.chromium.org/375253002/diff/100001/ppapi/proxy/printing_re... File ppapi/proxy/printing_resource.cc (right): https://codereview.chromium.org/375253002/diff/100001/ppapi/proxy/printing_re... ppapi/proxy/printing_resource.cc:50: PpapiHostMsg_PrintHost_SetPrintPresetOptionsFromDocument( why do you use Post here and not Call<> as above?
vitalybuka@ - Thanks for taking a look at this! Please see my response to your comments. https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_printing_renderer_host.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_printing_renderer_host.cc:56: PrintWebViewHelper* print_view_helper = PrintWebViewHelper::Get(render_view); On 2014/09/03 21:36:24, Vitaly Buka wrote: > why not to send message to browser directly, without using helper? Yea, we can send from here without going to helper. But it's not working. Please see my other comment. https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1015: PrintHostMsg_SetOptionsFromDocument_Params params; On 2014/09/03 21:36:24, Vitaly Buka wrote: > Why this one is not enough? becase of out-of-process? > So scaling also didn't work before? This one is enough when we use PPP interfaces i.e. browser to plugin. During PrintWebViewHelper::OnPrintPreview(), we request options from plugin and set them by sending message to browser. So scaling works since it uses PPP interface. I used this approach in initial patch for NumCopies. But now that we have moved to PPB interface i.e. plugin to browser for this patch, using same IPC again doesn't seem to work. Current patch uses following approach - [1] Set options from document in Instance::DocumentLoadComplete() (https://codereview.chromium.org/375253002/diff/100001/pdf/instance.cc) [2] Send these options through PPB interface to PepperPrintingRendererHost::OnHostMsgSetPrintPresetOptionsFromDocument (added in this patch). [3] Inform PrintPreviewUI to update settings. (https://codereview.chromium.org/375253002/diff2/80001:100001/chrome/renderer/...) The step [3] is giving problem. When I send PrintHostMsg_SetOptionsFromDocument IPC message, nothing happens. I've tried sending this IPC from helper and from PepperPrintingRendererHost to no avail. I'm suspecting the interaction model might be incorrect. FWIW, I can send ChromeViewHostMsg just fine. I tried sending ChromeViewHostMsg to chrome/browser/renderer_host/ from PepperPrintingRendererHost. That is browser thread I think, but I was unable to get WebContents required to get PrintPreviewUI. Hope the above makes some sense. Please let me know WDYT. Also, please see (maybe relevant?) - https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/flash_... https://codereview.chromium.org/375253002/diff/100001/ppapi/proxy/printing_re... File ppapi/proxy/printing_resource.cc (right): https://codereview.chromium.org/375253002/diff/100001/ppapi/proxy/printing_re... ppapi/proxy/printing_resource.cc:50: PpapiHostMsg_PrintHost_SetPrintPresetOptionsFromDocument( On 2014/09/03 21:36:24, Vitaly Buka wrote: > why do you use Post here and not Call<> as above? Call<> expects a response and a callback should be provided that will be called when response is received. Whereas, Post() just sends IPC messages and doesn't expect a reply. Please see - https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin... In our case, we want to notify print preview to set print preset options based on options set in the document. So we don't care about the response. That is why I used Post(). Please let me know WDYT.
https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_printing_renderer_host.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_printing_renderer_host.cc:56: PrintWebViewHelper* print_view_helper = PrintWebViewHelper::Get(render_view); Also - another thing to keep in mind is that we could simplify this CL a lot if there is a way to set the number of copies in the browser directly. What we would do is switch to using the pepper_printing_host.cc that is already inside the browser. If we can set the number of copies directly from inside that file, then we don't have to add a new printing host in the renderer. It would be best to go down that road if it's possible. It may also help solve your problem (though I have no idea what's actually causing the problem you're seeing).
https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_printing_renderer_host.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_printing_renderer_host.cc:56: PrintWebViewHelper* print_view_helper = PrintWebViewHelper::Get(render_view); Also - another thing to keep in mind is that we could simplify this CL a lot if there is a way to set the number of copies in the browser directly. What we would do is switch to using the pepper_printing_host.cc that is already inside the browser. If we can set the number of copies directly from inside that file, then we don't have to add a new printing host in the renderer. It would be best to go down that road if it's possible. It may also help solve your problem (though I have no idea what's actually causing the problem you're seeing).
please also note other nits from my previous comment https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/375253002/diff/100001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1015: PrintHostMsg_SetOptionsFromDocument_Params params; Discussed in chat. On 2014/09/04 11:42:53, Nikhil wrote: > On 2014/09/03 21:36:24, Vitaly Buka wrote: > > Why this one is not enough? becase of out-of-process? > > So scaling also didn't work before? > > This one is enough when we use PPP interfaces i.e. browser to plugin. During > PrintWebViewHelper::OnPrintPreview(), we request options from plugin and set > them by sending message to browser. So scaling works since it uses PPP > interface. I used this approach in initial patch for NumCopies. > > But now that we have moved to PPB interface i.e. plugin to browser for this > patch, using same IPC again doesn't seem to work. Current patch uses following > approach - > > [1] Set options from document in Instance::DocumentLoadComplete() > (https://codereview.chromium.org/375253002/diff/100001/pdf/instance.cc) > > [2] Send these options through PPB interface to > PepperPrintingRendererHost::OnHostMsgSetPrintPresetOptionsFromDocument (added in > this patch). > > [3] Inform PrintPreviewUI to update settings. > (https://codereview.chromium.org/375253002/diff2/80001:100001/chrome/renderer/...) > > The step [3] is giving problem. When I send PrintHostMsg_SetOptionsFromDocument > IPC message, nothing happens. I've tried sending this IPC from helper and from > PepperPrintingRendererHost to no avail. > > I'm suspecting the interaction model might be incorrect. > > FWIW, I can send ChromeViewHostMsg just fine. I tried sending ChromeViewHostMsg > to chrome/browser/renderer_host/ from PepperPrintingRendererHost. That is > browser thread I think, but I was unable to get WebContents required to get > PrintPreviewUI. > > Hope the above makes some sense. Please let me know WDYT. > > Also, please see (maybe relevant?) - > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/flash_...
n.bansal@samsung.com changed reviewers: + lazyboy@chromium.org
lazyboy@ - Could you please take a look at this? It will be really great if you can share some insights on this. The problem that I'm facing is that when I send IPC from PepperPrintingRendererHost::OnHostMsgSetPrintPresetOptionsFromDocument() [1], it doesn't reach browser at PrintPreviewUI::OnSetOptionsFromDocument [2]. [1] - https://codereview.chromium.org/375253002/diff/120001/chrome/renderer/pepper/... [2] - https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Thanks!
I suspect (but can't be sure) that this is a different issue from what lazyboy fixed as the issue he was addressing was related to interactions with BrowserPlugin. This bug is occurring without a BrowserPlugin being around at all.
raymes@ - Does Flash use PPP_PDF? Do we need to worry about backward compatibility with Flash if we change PPP_PDF interface?
lgtm
raymes@, dmichael@ - How about changing PPP_PDF interface? Does it also offer challenges similar to PPP_Printing_Dev? Do we need to worry about backward compatibility with Flash if we change PPP_PDF interface?
On 2014/09/19 06:28:07, Nikhil wrote: > raymes@, dmichael@ - How about changing PPP_PDF interface? Does it also offer > challenges similar to PPP_Printing_Dev? Do we need to worry about backward > compatibility with Flash if we change PPP_PDF interface? I haven't been watching this CL in a while, so I don't have context. But... it looks like Flash does *not* use PPB_Pdf, so we can assume it also doesn't use PPP_Pdf. So we don't need to worry about backwards-compatibility for PPB_Pdf or PPP_Pdf.
On 2014/09/19 15:41:31, dmichael wrote: > On 2014/09/19 06:28:07, Nikhil wrote: > > raymes@, dmichael@ - How about changing PPP_PDF interface? Does it also offer > > challenges similar to PPP_Printing_Dev? Do we need to worry about backward > > compatibility with Flash if we change PPP_PDF interface? > I haven't been watching this CL in a while, so I don't have context. > > But... it looks like Flash does *not* use PPB_Pdf, so we can assume it also > doesn't use PPP_Pdf. So we don't need to worry about backwards-compatibility for > PPB_Pdf or PPP_Pdf. Thanks, David! This is great. I faced certain issues with PPB_Pdf/PPB_Printing_Dev, but I think I can make it work with PPP_Pdf. I'm going to it a try :)
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
n.bansal@samsung.com changed reviewers: + thestig@chromium.org
vitalybuka@, raymes@, dmichael@, thestig@ - Please provide your blessings :) I got this working with PPP_Pdf changes. I tested with PrintPreviewUI changes as well and copies is getting updated properly in print preview dialog. In addition to this CL, following two CLs are also needed to support this feature : [1] Blink changes - https://codereview.chromium.org/591053002/ [2] JS changes - https://codereview.chromium.org/407733002/ Please take a look. Thanks
The big thing that's missing here is the out-of-process proxy. Please see: ppapi/proxy/ppp_pdf_proxy.* ppapi/proxy/ppapi_messages.h https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:252: blink::WebPrintPresetOptions& preset_options); nit: In Chromium, out-params should be by pointer. See: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... (In general, follow: http://www.chromium.org/developers/coding-style and http://google-styleguide.googlecode.com/svn/trunk/cppguide.html ) https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_webplugin_impl.h (right): https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_webplugin_impl.h:84: blink::WebPrintPresetOptions& preset_options); OVERRIDE? Or does that have to wait for the blink change to land? https://codereview.chromium.org/375253002/diff/180001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/180001/pdf/instance.cc#newcode224 pdf/instance.cc:224: ->GetPrintPresetOptionsFromDocument(options); The style here looks funny. I think normally we'd have -> on the previous line, and the 4-space indent would probably be count from where "ret" starts. How about just having a "Instance* obj_instance" stack variable, like Transform does? https://codereview.chromium.org/375253002/diff/180001/pdf/instance.h File pdf/instance.h (right): https://codereview.chromium.org/375253002/diff/180001/pdf/instance.h#newcode94 pdf/instance.h:94: PP_PrintPresetOptions_Dev& options); same nit; out-params should be by pointer https://codereview.chromium.org/375253002/diff/180001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/375253002/diff/180001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:172: ->GetPrintPresetOptionsFromDocument(options); ditto, style looks off. just make an obj_instance variable. Also, please use curly braces for an if that takes multiple lines, even if it's only 1 expression. https://codereview.chromium.org/375253002/diff/180001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/180001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:13: #define PPP_PDF_INTERFACE_2 "PPP_Pdf;2" For something this small we would usually go to 1.1 or something.
Patchset #8 (id:200001) has been deleted
dmichael@ - Thanks for reviewing this! I've modified the code as per review comments. PTAL. Thanks. https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:252: blink::WebPrintPresetOptions& preset_options); On 2014/09/22 18:11:51, dmichael wrote: > nit: In Chromium, out-params should be by pointer. > See: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... > > (In general, follow: > http://www.chromium.org/developers/coding-style > and > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html > ) Thanks for the links :) I've updated the code. PTAL. https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_webplugin_impl.h (right): https://codereview.chromium.org/375253002/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_webplugin_impl.h:84: blink::WebPrintPresetOptions& preset_options); On 2014/09/22 18:11:51, dmichael wrote: > OVERRIDE? Or does that have to wait for the blink change to land? I think OVERRIDE isn't required. Please see linkAtPosition() that is used for GetLinkAtPosition() API of PPP_Pdf interface. I've updated code to put new method close to linkAtPosition() to make this look consistent. https://codereview.chromium.org/375253002/diff/180001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/180001/pdf/instance.cc#newcode224 pdf/instance.cc:224: ->GetPrintPresetOptionsFromDocument(options); On 2014/09/22 18:11:51, dmichael wrote: > The style here looks funny. I think normally we'd have -> on the previous line, > and the 4-space indent would probably be count from where "ret" starts. > > How about just having a "Instance* obj_instance" stack variable, like Transform > does? The style here was a result of "git cl format". I've modified the code to make use of stack variable. https://codereview.chromium.org/375253002/diff/180001/pdf/instance.h File pdf/instance.h (right): https://codereview.chromium.org/375253002/diff/180001/pdf/instance.h#newcode94 pdf/instance.h:94: PP_PrintPresetOptions_Dev& options); On 2014/09/22 18:11:51, dmichael wrote: > same nit; out-params should be by pointer Done. https://codereview.chromium.org/375253002/diff/180001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/375253002/diff/180001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:172: ->GetPrintPresetOptionsFromDocument(options); On 2014/09/22 18:11:51, dmichael wrote: > ditto, style looks off. just make an obj_instance variable. > > Also, please use curly braces for an if that takes multiple lines, even if it's > only 1 expression. Done. https://codereview.chromium.org/375253002/diff/180001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/180001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:13: #define PPP_PDF_INTERFACE_2 "PPP_Pdf;2" On 2014/09/22 18:11:51, dmichael wrote: > For something this small we would usually go to 1.1 or something. Done.
dmichael@chromium.org changed reviewers: + tsepez@chromium.org
I might have preferred Raymes's suggestion of a "SetPrintOptions" on PPB_Pdf vs "GetPrintOptions" on PPP_Pdf, just because the former would have avoided using a sync message I think. But I can't think of a problem with this way, and it is nice and simple! so lgtm +tsepez for ppapi_messages.h https://codereview.chromium.org/375253002/diff/220001/content/renderer/pepper... File content/renderer/pepper/pepper_webplugin_impl.h (right): https://codereview.chromium.org/375253002/diff/220001/content/renderer/pepper... content/renderer/pepper/pepper_webplugin_impl.h:72: blink::WebPrintPresetOptions* preset_options); Please keep the methods in the same order as they are in the base class, so put it back after isPrintScalingDisabled(). As for OVERRIDE, it's not strictly necessary for things we override from blink, but I usually prefer to see it just for the sake of documenting it. Since we're inconsistent here right now, I'm fine with leaving it off.
Messages LGTM.
Thanks David, Tom for reviewing this! raymes@, thestig@ - PTAL at pdf/* changes. vitalybuka@ - PTAL at chrome/renderer/printing/print_web_view_helper.cc changes.
On 2014/09/23 18:54:56, Nikhil wrote: > Thanks David, Tom for reviewing this! > > raymes@, thestig@ - PTAL at pdf/* changes. > > vitalybuka@ - PTAL at chrome/renderer/printing/print_web_view_helper.cc changes. print_web_view_helper.cc and pdf lgtm, but let raymes takes a look too.
A few comments. It's a bit unfortunate to put this in PPP_Pdf but given all the other constraints here I think it's ok! https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc#newcode672 pdf/instance.cc:672: return PP_TRUE; If the return value is always PP_TRUE, we might as well get rid of it? https://codereview.chromium.org/375253002/diff/220001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:564: return PP_TRUE; Same here. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... File ppapi/c/dev/pp_print_settings_dev.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:86: struct PP_PrintPresetOptions_Dev { I'd prefer to move this struct into PPP_Pdf (and name it appropriately as PP_PdfPrintPresetOptions or something). The reason being that it makes it clear that we can change the struct at anytime without worrying about flash. When people look at this file they have to worry about backward compatibility with flash but that's not the case with PDF. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:87: PP_Bool is_scaling_disabled; Since we already have a PPP function to get whether scaling is disabled (in PPP_Printing) I think it's better to leave this out too (otherwise it becomes unclear which value is actually being used). I'm ok with keeping the struct with just 1 member for now. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:91: int32_t page_range_count; Are the last 3 members of the struct used at all? Should we remove them? https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:13: #define PPP_PDF_INTERFACE_1_1 "PPP_Pdf;1.1" AFAIK, there is no real reason to have to change the version number, so I would just leave it at 1
raymes@ - Sorry for the delay! Please see my response to your comments. I'll update the patch accordingly based on your response. Thank you for reviewing this :) https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc#newcode672 pdf/instance.cc:672: return PP_TRUE; On 2014/09/24 04:54:39, raymes wrote: > If the return value is always PP_TRUE, we might as well get rid of it? I think it can be removed. I'm not sure about behavior for page_range so I maintained bool return type for now. But since there seems to be no backward compatibility or versioning issues with PPP_Pdf then maybe make void now? And change to bool, if required when adding support for page_range? This affects blink changes though. Please let me know WDYT. https://codereview.chromium.org/375253002/diff/220001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:564: return PP_TRUE; On 2014/09/24 04:54:39, raymes wrote: > Same here. Acknowledged. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... File ppapi/c/dev/pp_print_settings_dev.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:87: PP_Bool is_scaling_disabled; On 2014/09/24 04:54:39, raymes wrote: > Since we already have a PPP function to get whether scaling is disabled (in > PPP_Printing) I think it's better to leave this out too (otherwise it becomes > unclear which value is actually being used). I'm ok with keeping the struct with > just 1 member for now. Pdf print preset requires four properties to be supported - [1] Print scaling [2] Number of copies [3] Duplex mode [4] Print range I added all four in this struct to make it complete. [1] is already supported by PPP_Printing function. So, if we leave out scaling, we would be using both PPP_Printing and PPP_Pdf for this. And PrintWebViewHelper::SetOptionsFromDocument() would need to be modified to call both interfaces. Please see : https://codereview.chromium.org/375253002/diff/220001/chrome/renderer/printin... Is that okay? Or maybe add a note that both values are same for scaling? https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:91: int32_t page_range_count; On 2014/09/24 04:54:39, raymes wrote: > Are the last 3 members of the struct used at all? Should we remove them? They are not used right now. But I intend to support them in future CLs. I wasn't sure about backward compatibility changes so I added all required fields in this CL. But since there are no concerns with backward compatibility, I'll remove them for now :)
raymes@ - <ping!> :)
raymes@chromium.org changed reviewers: - thestig@chromium.org
lgtm assuming the struct gets moved to PPP_PDF Thanks! https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc#newcode672 pdf/instance.cc:672: return PP_TRUE; If you want to keep the PP_Bool in the API, that's ok but this part isn't in the API so we could just remove the return value here for now and always return PP_TRUE from GetPrintPresetOptionsFromDocument above. It's a minor thing. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... File ppapi/c/dev/pp_print_settings_dev.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:86: struct PP_PrintPresetOptions_Dev { On 2014/09/24 04:54:39, raymes wrote: > I'd prefer to move this struct into PPP_Pdf (and name it appropriately as > PP_PdfPrintPresetOptions or something). The reason being that it makes it clear > that we can change the struct at anytime without worrying about flash. When > people look at this file they have to worry about backward compatibility with > flash but that's not the case with PDF. This part hasn't been addressed, it would be better to move the struct I think. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:87: PP_Bool is_scaling_disabled; Ok I think you're right that it's clearer that everything is one interface. But please put a comment in the API saying that it should return the same information as the PPP function. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:91: int32_t page_range_count; On 2014/09/29 11:15:30, Nikhil wrote: > On 2014/09/24 04:54:39, raymes wrote: > > Are the last 3 members of the struct used at all? Should we remove them? > > They are not used right now. But I intend to support them in future CLs. > > I wasn't sure about backward compatibility changes so I added all required > fields in this CL. But since there are no concerns with backward compatibility, > I'll remove them for now :) It's ok to keep them if they will be used, I didn't realize. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:13: #define PPP_PDF_INTERFACE_1_1 "PPP_Pdf;1.1" On 2014/09/24 04:54:39, raymes wrote: > AFAIK, there is no real reason to have to change the version number, so I would > just leave it at 1 Just a reminder to keep this at 1
raymes@ - Sorry for the delay! I've moved the struct to PPP_Pdf and updated the patch accordingly. PTAL :) Thanks! https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/375253002/diff/220001/pdf/instance.cc#newcode672 pdf/instance.cc:672: return PP_TRUE; On 2014/10/01 17:26:23, raymes wrote: > If you want to keep the PP_Bool in the API, that's ok but this part isn't in the > API so we could just remove the return value here for now and always return > PP_TRUE from GetPrintPresetOptionsFromDocument above. It's a minor thing. Done. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... File ppapi/c/dev/pp_print_settings_dev.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:86: struct PP_PrintPresetOptions_Dev { On 2014/10/01 17:26:23, raymes wrote: > On 2014/09/24 04:54:39, raymes wrote: > > I'd prefer to move this struct into PPP_Pdf (and name it appropriately as > > PP_PdfPrintPresetOptions or something). The reason being that it makes it > clear > > that we can change the struct at anytime without worrying about flash. When > > people look at this file they have to worry about backward compatibility with > > flash but that's not the case with PDF. > > This part hasn't been addressed, it would be better to move the struct I think. Done. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:87: PP_Bool is_scaling_disabled; On 2014/10/01 17:26:23, raymes wrote: > Ok I think you're right that it's clearer that everything is one interface. But > please put a comment in the API saying that it should return the same > information as the PPP function. Done. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/dev/pp_print_se... ppapi/c/dev/pp_print_settings_dev.h:91: int32_t page_range_count; On 2014/10/01 17:26:23, raymes wrote: > On 2014/09/29 11:15:30, Nikhil wrote: > > On 2014/09/24 04:54:39, raymes wrote: > > > Are the last 3 members of the struct used at all? Should we remove them? > > > > They are not used right now. But I intend to support them in future CLs. > > > > I wasn't sure about backward compatibility changes so I added all required > > fields in this CL. But since there are no concerns with backward > compatibility, > > I'll remove them for now :) > > It's ok to keep them if they will be used, I didn't realize. Acknowledged. https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/220001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:13: #define PPP_PDF_INTERFACE_1_1 "PPP_Pdf;1.1" On 2014/10/01 17:26:23, raymes wrote: > On 2014/09/24 04:54:39, raymes wrote: > > AFAIK, there is no real reason to have to change the version number, so I > would > > just leave it at 1 > > Just a reminder to keep this at 1 Done.
lgtm with 1 name change https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:24: struct PPP_PdfPrintPresetOptions_Dev { This should be called PP_PdfPrintPresetOptions_Dev (not PPP_...)
raymes@ - Thanks for taking a look! I've updated the name of struct based on your suggestion. PTAL. I'll update Blink patch now since this CL is pretty much ready :) https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf.h File ppapi/c/private/ppp_pdf.h (right): https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf... ppapi/c/private/ppp_pdf.h:24: struct PPP_PdfPrintPresetOptions_Dev { On 2014/10/07 17:27:13, raymes wrote: > This should be called PP_PdfPrintPresetOptions_Dev (not PPP_...) Done.
On 2014/10/08 06:51:50, Nikhil wrote: > raymes@ - Thanks for taking a look! I've updated the name of struct based on > your suggestion. PTAL. > > I'll update Blink patch now since this CL is pretty much ready :) > > https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf.h > File ppapi/c/private/ppp_pdf.h (right): > > https://codereview.chromium.org/375253002/diff/240001/ppapi/c/private/ppp_pdf... > ppapi/c/private/ppp_pdf.h:24: struct PPP_PdfPrintPresetOptions_Dev { > On 2014/10/07 17:27:13, raymes wrote: > > This should be called PP_PdfPrintPresetOptions_Dev (not PPP_...) > > Done. lgtm
raymes@, dmichael@ - Sorry for the delay in getting back to this! Could you please take a quick look at this? I request to take a look again as it has been a while since I got all the required LGTMs for this CL :) The corresponding blink patch (https://codereview.chromium.org/591053002/) has landed. I've also tested the changes along with UI changes (https://codereview.chromium.org/407733002/) and it's working fine. Thank you for bearing with me for past few months and explaining so many things! I think we can submit this now. Please take a look. Thanks!
Have you made any changes besides rebasing? If not it's ok to go ahead and land.
On 2014/11/13 22:03:25, raymes wrote: > Have you made any changes besides rebasing? If not it's ok to go ahead and land. Great, I'll land this after verifying the blink roll. I've not done any other changes, just the rebase :)
Thank you all for reviewing this! CQing this guy now :)
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/375253002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/87188)
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/375253002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/87236)
On 2014/11/17 13:37:33, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/87236) raymes@ - I've made one change to fix the warning on win_gpu bot. Please take a look. Thanks!
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/375253002/370001
Message was sent while issue was closed.
Committed patchset #15 (id:370001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/64c4daa7d64fc28efd1e49bf3548323eb67a32ec Cr-Commit-Position: refs/heads/master@{#304785} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
