Hookup print and save buttons in the out of process PDF plugin

Hookup print and save buttons in the out of process PDF plugin

BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252884

[raymes, 2014-02-18 01:36:12]

As usual please ignore the generated index.html/index.js

[koz (OOO until 15th September), 2014-02-18 05:15:07]

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/background.js:7: function getStreamDetails() {
nit: It's slightly odd that a get* function has side effects. Consider renaming
to popStreamDetails()?

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/background.js:10: return streamDetails;
nit: just return _streamsCache.pop();

Also: why do we need a stack of stream details? Could you add a comment
explaining why.

[raymes, 2014-02-19 00:22:15]

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/background.js:7: function getStreamDetails() {
On 2014/02/18 05:15:07, koz wrote:
> nit: It's slightly odd that a get* function has side effects. Consider
renaming
> to popStreamDetails()?

Done.

https://codereview.chromium.org/169613005/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/background.js:10: return streamDetails;
On 2014/02/18 05:15:07, koz wrote:
> nit: just return _streamsCache.pop();
> 
> Also: why do we need a stack of stream details? Could you add a comment
> explaining why.

Done.

[koz (OOO until 15th September), 2014-02-19 01:52:10]

https://codereview.chromium.org/169613005/diff/110001/chrome/browser/resource...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/110001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:9: * order that the requests come in.
Won't this mean that the requests get handled in reverse order? Is that what you
intend?

[raymes, 2014-02-19 03:56:15]

On 2014/02/19 01:52:10, koz wrote:
>
https://codereview.chromium.org/169613005/diff/110001/chrome/browser/resource...
> File chrome/browser/resources/pdf/background.js (right):
> 
>
https://codereview.chromium.org/169613005/diff/110001/chrome/browser/resource...
> chrome/browser/resources/pdf/background.js:9: * order that the requests come
in.
> Won't this mean that the requests get handled in reverse order? Is that what
you
> intend?

...oops yeah that's true, I took this code from quickoffice. The ordering isn't
really important anyway as the thing is probably racy. I've added a TODO but
this code will probably change anyway so I won't fix it just yet.

[koz (OOO until 15th September), 2014-02-20 00:55:02]

Sorry, I forgot I hadn't lgtm'd yet!

[raymes, 2014-02-20 00:56:50]

+arv for OWNERS

[arv (Not doing code reviews), 2014-02-20 20:10:44]

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:6: * @private
Use an IIFE

(function() {
  'use strict';

  var streamsCache = [];

  ...
})();

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:14: var streamsCache_ = [];
No underscores on variables.

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/pdf/index.html (right):

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/index.html:424: <a id="save-button-link"
download="">
<a id="save-button-link" download>

[raymes, 2014-02-21 03:52:23]

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:6: * @private
On 2014/02/20 20:10:45, arv wrote:
> Use an IIFE
> 
> (function() {
>   'use strict';
> 
>   var streamsCache = [];
> 
>   ...
> })();

Done.

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:14: var streamsCache_ = [];
On 2014/02/20 20:10:45, arv wrote:
> No underscores on variables.

Done.

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/pdf/index.html (right):

https://codereview.chromium.org/169613005/diff/210001/chrome/browser/resource...
chrome/browser/resources/pdf/index.html:424: <a id="save-button-link"
download="">
On 2014/02/20 20:10:45, arv wrote:
> <a id="save-button-link" download>

vulcanize does this, you can see that the original index.in.html is correct.

[arv (Not doing code reviews), 2014-02-21 14:41:03]

LGTM

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:21: }
missing semicolon now that is a statement.

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:24: function(mime_type, original_url,
content_url, tab_id) {
no underscores in js

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:34: chrome.tabs.update(tab_id, { url:
'index.html' });
no ws after { nor before }

[raymes, 2014-02-23 23:55:10]

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
File chrome/browser/resources/pdf/background.js (right):

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:21: }
On 2014/02/21 14:41:03, arv wrote:
> missing semicolon now that is a statement.

Done.

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:24: function(mime_type, original_url,
content_url, tab_id) {
On 2014/02/21 14:41:03, arv wrote:
> no underscores in js

Done.

https://codereview.chromium.org/169613005/diff/330001/chrome/browser/resource...
chrome/browser/resources/pdf/background.js:34: chrome.tabs.update(tab_id, { url:
'index.html' });
On 2014/02/21 14:41:03, arv wrote:
> no ws after { nor before }

Done.

[raymes, 2014-02-23 23:55:34]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-02-23 23:55:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/169613005/390001

[commit-bot: I haz the power, 2014-02-24 06:07:51]

Change committed as 252884