-
Notifications
You must be signed in to change notification settings - Fork 652
Enable window.print() for Linux #3851
base: master
Are you sure you want to change the base?
Conversation
Testing patch series with mrunalk/crosswalk@5abc149 as its head.
|
Testing patch series with mrunalk/crosswalk@36ffa22 as its head.
|
36ffa22
to
d45a698
Compare
Testing patch series with mrunalk/crosswalk@d45a698 as its head.
|
This enables window.print() api so that crosswalk based apps can print within their app. It doesn't provide Print Preview like Chrome but does pop up system print dialog to configure print settings. It also relies on a small modification to build/common.gypi in chromium-crosswalk repo to disable the print preview for which I will submit a seprate PR. BUG=XWALK-5680
d45a698
to
e8bc8e3
Compare
Testing patch series with mrunalk/crosswalk@e8bc8e3 as its head.
|
@darktears PTAL. |
Testing patch series with mrunalk/crosswalk@3c27e36 as its head.
|
I'm not sure removing the Win specific part is a good idea right now. You're going to break potential users (not sure if we have any but still). If the code was landed in the first place it's because it was a request. |
@darktears that's just a small piece of what Windows needs which I have removed here so that it will be easier on reviews. I'm working on Windows implementation in parallel which needs additional code. I'm also aware that making it work on Windows is the ultimate goal here. |
as soon as you're confident that there will not be breakage (aka you land all the pieces before it reaches stable) then that's ok. Other question : it seems that a bunch of code is copied from somewhere else, where is the origin? |
Yes I should be able to land the Windows implementation before crosswalk 23 branches into beta. Lot of it is copied from Chromium's browser layer. All of the classes in runtime/browser/printing are copied from Chromium's chrome/browser/printing and adapted for Crosswalk. XWalkPrintWebViewHelperDelegate has been adapted from AwPrintWebViewHelperDelegate. Rest of the code in Crosswalk follows the flow and structure in Chromium. May be I should mention this as part of commit message. |
@darktears ping |
I wish we could avoid copying all of that...long term it's going to be a nightmare to make sure we back port fixes. Out of curiosity how hard was it to extract it from chrome/? Many classes seemed like a plain copy paste. Maybe it could go into components/printing? |
I agree, I wish we could avoid copying the files in runtime/browser/printing too but since most of it is implemented in chrome I had to extract the minimum functionality from there to crosswalk. There is no point in reinventing the wheel here. May be some of it could be moved to components/printing but I don't see much progress upstream on any of the issues, 465802, 172158. Extracting the classes in runtime/browser/printing wasn't that difficult but there was quite a bit of work involved in adapting it in crosswalk code and making sure we are initializing and freeing up resources properly in line with chromium flow. I feel componentizing printing will require lot of effort because 1. it depends on system api's 2. There is lot of code for preview which requires UI 3. Cloud print is big component of it. Regarding backporting fixes it may seem lot of code to rebase but I don't think printing code changes that much. |
Answering few of your comments :
|
Maybe...but that is outside the scope of this PR for now as it will take some time to implement it upstream. I am not sure if I can commit to it now. Current goal is to enable printing for our users with what we have in crosswalk and chromium.
Ok, may be that's possible then.
I don't quite understand what you mean here. If you are suggesting to componentize the printing functionality except cloud print first and then cloud print next, I agree with you.
Yes, it's possible we may miss some of the fixes done upstream. I don't think anyone can guarantee that with every rebase unless we componentize printing completely. As long as we have printing functionality working correctly for our users that is fine IMO. |
I don't have strong opinion, it's just that we need to be careful on what we land and the cost is has considering the discussions ongoing. |
I think that question can be answered by @baleboy well. I am just responding to the needs conveyed to me by enabling the functionality. There were some customers who needed the print functionality on Android at one point(I don't want to name them here). But with our changing priorities I shifted the focus to Windows by starting with Linux implementation. After Windows I can also provide the implementation for Android. There is no pressure here as such but I want this basic implementation to pass the review or atleast get a positive signal so that I can move ahead on the Windows implementation. Otherwise it will just be a waste of time.
Once again, I don't plan to work on componentizing it as of now.
From my experience working on refactoring Speech API upstream it can also take months to pass reviews and land all the patches upstream. Again, I don't plan to work on upstreaming it as of now. |
This enables window.print() api so that crosswalk based apps can print within their app.
It doesn't provide Print Preview like Chrome but does pop up system print dialog to
configure print settings. It also relies on a small modification to build/common.gypi
in chromium-crosswalk repo to disable the print preview for which I will submit a
seprate PR.
BUG=XWALK-5680