[CUPS] Fix the issue the default queue value not being persisted with each new printer setup.

[CUPS] Fix the issue the default queue value not being persisted with each new printer setup.

BUG=710661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2860883004
Cr-Commit-Position: refs/heads/master@{#469825}
Committed: https://chromium.googlesource.com/chromium/src/+/504e47cd10aa741e9315fdd02d8092cb5101dbff

[xdai1, 2017-05-04 20:33:29]

Description was changed from

==========
[CUPS] Fix the issue the default queue value not being persisted with each new
printer setup.

BUG=710661
==========

to

==========
[CUPS] Fix the issue the default queue value not being persisted with each new
printer setup.

BUG=710661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[xdai1, 2017-05-04 20:35:58]

xdai@chromium.org changed reviewers:
+ michaelpg@chromium.org

[xdai1, 2017-05-04 20:35:59]

michaelpg@, could you help review this CL please? Thanks!

https://codereview.chromium.org/2860883004/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html
(right):

https://codereview.chromium.org/2860883004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:195:
label="ipp/print"
the label text was hard-coded since it doesn't require translation.

[xdai1, 2017-05-04 20:36:14]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:36:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2017-05-04 22:30:10]

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
this.newPrinter.printerQueue = "ipp/print";
here and elsewhere, why does this work? Normally Polymer would need to be
notified of the change for data binding to update. Since this is setting a
subproperty directly instead of with this.set(), I'd expect the UI to not get
updated with the new value.

Maybe it's okay here because we're closing the dialog, but what about in
onProtocolChange_ below?

[xdai1, 2017-05-05 00:45:37]

michaelpg@, I've addressed your comment. Please take another look, thanks!

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
this.newPrinter.printerQueue = "ipp/print";
On 2017/05/04 22:30:10, michaelpg wrote:
> here and elsewhere, why does this work? Normally Polymer would need to be
> notified of the change for data binding to update. Since this is setting a
> subproperty directly instead of with this.set(), I'd expect the UI to not get
> updated with the new value.
> 
> Maybe it's okay here because we're closing the dialog, but what about in
> onProtocolChange_ below?

I think the root reason is it means I don't fully understand how Polymer data
binding works... But for this specific case, I think the reason it works here
and in onProtocolChange_() is there is no need of UI update for printerQueue and
printerProtocol.

I modified all the places that modify a subproperty to this.set(...).

[michaelpg, 2017-05-05 19:10:46]

lgtm

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
this.newPrinter.printerQueue = "ipp/print";
On 2017/05/05 00:45:37, xdai1 wrote:
> On 2017/05/04 22:30:10, michaelpg wrote:
> > here and elsewhere, why does this work? Normally Polymer would need to be
> > notified of the change for data binding to update. Since this is setting a
> > subproperty directly instead of with this.set(), I'd expect the UI to not
get
> > updated with the new value.
> > 
> > Maybe it's okay here because we're closing the dialog, but what about in
> > onProtocolChange_ below?
> 
> I think the root reason is it means I don't fully understand how Polymer data
> binding works... 

It's black magic, none of us do :-)

But generally, Polymer automatically notices and propagates a change *only* if
the property being set is a top-level Polymer property, *or* if you call
this.set/this.notifyPath.
https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path

Polymer properties, like this.newPrinter, are setters/getters. When you "set"
this.newPrinter = foo, you actually call a Polymer method, which takes care of
updating the backing "newPrinter" value *and* notifies observers and bindings
that the value of this.newPrinter is changed.

When you update a sub-property, like this.newPrinter.bar = foo, Polymer doesn't
know. this.newPrinter is just a getter, so it returns the real "newPrinter"
object. Setting a property of that object doesn't trigger any magic because it's
just a plain old JavaScript object.

> But for this specific case, I think the reason it works here
> and in onProtocolChange_() is there is no need of UI update for printerQueue
and
> printerProtocol.
> 
> I modified all the places that modify a subproperty to this.set(...).

[xdai1, 2017-05-05 20:32:49]

On 2017/05/05 19:10:46, michaelpg wrote:
> lgtm
> 
>
https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
> File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
> (right):
> 
>
https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
> this.newPrinter.printerQueue = "ipp/print";
> On 2017/05/05 00:45:37, xdai1 wrote:
> > On 2017/05/04 22:30:10, michaelpg wrote:
> > > here and elsewhere, why does this work? Normally Polymer would need to be
> > > notified of the change for data binding to update. Since this is setting a
> > > subproperty directly instead of with this.set(), I'd expect the UI to not
> get
> > > updated with the new value.
> > > 
> > > Maybe it's okay here because we're closing the dialog, but what about in
> > > onProtocolChange_ below?
> > 
> > I think the root reason is it means I don't fully understand how Polymer
data
> > binding works... 
> 
> It's black magic, none of us do :-)
> 
> But generally, Polymer automatically notices and propagates a change *only* if
> the property being set is a top-level Polymer property, *or* if you call
> this.set/this.notifyPath.
> https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path
> 
> Polymer properties, like this.newPrinter, are setters/getters. When you "set"
> this.newPrinter = foo, you actually call a Polymer method, which takes care of
> updating the backing "newPrinter" value *and* notifies observers and bindings
> that the value of this.newPrinter is changed.
> 
> When you update a sub-property, like this.newPrinter.bar = foo, Polymer
doesn't
> know. this.newPrinter is just a getter, so it returns the real "newPrinter"
> object. Setting a property of that object doesn't trigger any magic because
it's
> just a plain old JavaScript object.

Thank you very much for so detailed explanation!! Updating a sub-property using
this.newPrinter.bar = foo only updates the backing "newPrinter" value but
doesn't notify observers. But this.set(..) does both. So whenever possible, we
should use this.set(..) to update a sub-property. right?

> 
> > But for this specific case, I think the reason it works here
> > and in onProtocolChange_() is there is no need of UI update for printerQueue
> and
> > printerProtocol.
> > 
> > I modified all the places that modify a subproperty to this.set(...).

[michaelpg, 2017-05-05 20:45:50]

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
this.newPrinter.printerQueue = "ipp/print";
On 2017/05/05 19:10:46, michaelpg wrote:
> On 2017/05/05 00:45:37, xdai1 wrote:
> > On 2017/05/04 22:30:10, michaelpg wrote:
> > > here and elsewhere, why does this work? Normally Polymer would need to be
> > > notified of the change for data binding to update. Since this is setting a
> > > subproperty directly instead of with this.set(), I'd expect the UI to not
> get
> > > updated with the new value.
> > > 
> > > Maybe it's okay here because we're closing the dialog, but what about in
> > > onProtocolChange_ below?
> > 
> > I think the root reason is it means I don't fully understand how Polymer
data
> > binding works... 
> 
> It's black magic, none of us do :-)
> 
> But generally, Polymer automatically notices and propagates a change *only* if
> the property being set is a top-level Polymer property, *or* if you call
> this.set/this.notifyPath.
> https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path
> 
> Polymer properties, like this.newPrinter, are setters/getters. When you "set"
> this.newPrinter = foo, you actually call a Polymer method, which takes care of
> updating the backing "newPrinter" value *and* notifies observers and bindings
> that the value of this.newPrinter is changed.
> 
> When you update a sub-property, like this.newPrinter.bar = foo, Polymer
doesn't
> know. this.newPrinter is just a getter, so it returns the real "newPrinter"
> object. Setting a property of that object doesn't trigger any magic because
it's
> just a plain old JavaScript object.
> 
> > But for this specific case, I think the reason it works here
> > and in onProtocolChange_() is there is no need of UI update for printerQueue
> and
> > printerProtocol.
> > 
> > I modified all the places that modify a subproperty to this.set(...).
> 


Replying to your message:

> Thank you very much for so detailed explanation!! Updating a sub-property
using
> this.newPrinter.bar = foo only updates the backing "newPrinter" value but
> doesn't notify observers. But this.set(..) does both. So whenever possible, we
> should use this.set(..) to update a sub-property. right?

Basically, yes. Unless you can guarantee that it won't affect the UI, like if
you're always setting this.newPrinter to a completely new object.

You can still update a sub-property with normal data binding too:
  <paper-input value="{{newPrinter.foo}}">

[xdai1, 2017-05-05 22:19:23]

On 2017/05/05 20:45:50, michaelpg wrote:
>
https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
> File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
> (right):
> 
>
https://codereview.chromium.org/2860883004/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:160:
> this.newPrinter.printerQueue = "ipp/print";
> On 2017/05/05 19:10:46, michaelpg wrote:
> > On 2017/05/05 00:45:37, xdai1 wrote:
> > > On 2017/05/04 22:30:10, michaelpg wrote:
> > > > here and elsewhere, why does this work? Normally Polymer would need to
be
> > > > notified of the change for data binding to update. Since this is setting
a
> > > > subproperty directly instead of with this.set(), I'd expect the UI to
not
> > get
> > > > updated with the new value.
> > > > 
> > > > Maybe it's okay here because we're closing the dialog, but what about in
> > > > onProtocolChange_ below?
> > > 
> > > I think the root reason is it means I don't fully understand how Polymer
> data
> > > binding works... 
> > 
> > It's black magic, none of us do :-)
> > 
> > But generally, Polymer automatically notices and propagates a change *only*
if
> > the property being set is a top-level Polymer property, *or* if you call
> > this.set/this.notifyPath.
> > https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path
> > 
> > Polymer properties, like this.newPrinter, are setters/getters. When you
"set"
> > this.newPrinter = foo, you actually call a Polymer method, which takes care
of
> > updating the backing "newPrinter" value *and* notifies observers and
bindings
> > that the value of this.newPrinter is changed.
> > 
> > When you update a sub-property, like this.newPrinter.bar = foo, Polymer
> doesn't
> > know. this.newPrinter is just a getter, so it returns the real "newPrinter"
> > object. Setting a property of that object doesn't trigger any magic because
> it's
> > just a plain old JavaScript object.
> > 
> > > But for this specific case, I think the reason it works here
> > > and in onProtocolChange_() is there is no need of UI update for
printerQueue
> > and
> > > printerProtocol.
> > > 
> > > I modified all the places that modify a subproperty to this.set(...).
> > 
> 
> 
> Replying to your message:
> 
> > Thank you very much for so detailed explanation!! Updating a sub-property
> using
> > this.newPrinter.bar = foo only updates the backing "newPrinter" value but
> > doesn't notify observers. But this.set(..) does both. So whenever possible,
we
> > should use this.set(..) to update a sub-property. right?
> 
> Basically, yes. Unless you can guarantee that it won't affect the UI, like if
> you're always setting this.newPrinter to a completely new object.
> 
> You can still update a sub-property with normal data binding too:
>   <paper-input value="{{newPrinter.foo}}">

Got it. Thank you very much!

[xdai1, 2017-05-05 22:19:34]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2017-05-05 22:21:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 00:20:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494022774002530,
"parent_rev": "c8faec73855c122f94f00c54549ee6542da59e9b", "commit_rev":
"504e47cd10aa741e9315fdd02d8092cb5101dbff"}

[commit-bot: I haz the power, 2017-05-06 00:21:04]

Description was changed from

==========
[CUPS] Fix the issue the default queue value not being persisted with each new
printer setup.

BUG=710661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CUPS] Fix the issue the default queue value not being persisted with each new
printer setup.

BUG=710661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2860883004
Cr-Commit-Position: refs/heads/master@{#469825}
Committed:
https://chromium.googlesource.com/chromium/src/+/504e47cd10aa741e9315fdd02d80...
==========

[commit-bot: I haz the power, 2017-05-06 00:21:05]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/504e47cd10aa741e9315fdd02d80...