1114793 - Consider re-naming chrome window handle interactions for consistency

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Comment 1

[Chris Manchester (limited bugmail, email directly)]

:whimboo from bug 941749:

Chris, one thing I noticed here is the different implementation as shown below:

>   2.275    "getCurrentWindowHandle":  MarionetteServerConnection.prototype.getWindowHandle,  // Selenium 2 compat
>   2.276 +  "getChromeWindowHandle": MarionetteServerConnection.prototype.getChromeWindowHandle,

Was there a reason not to use getCurrentChromeWindowHandle together with .current_chrome_window_handle? Was it just the length which let you do the shorter property name? .chrome_window_handle doesn't really tell that it is the one for the currently selected chrome window.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1114793/chmanchester

Comment 3

[Chris Manchester (limited bugmail, email directly)]

/r/1657 - Bug 1114793 - Rename marionette's chrome_window_handle to current_chrome_window_handle.

Pull down this commit:

hg pull review -r e19acf0fe4d1065c0efed23e795795fb0aa3f4a2

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Henrik asked that I look into this. Both he and David are on holiday. I don't think renaming this is necessary, but if it's requested I certainly don't have a problem with it.

ato, if you are around this week perhaps you could lend an opinion on this matter? Thanks!

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Chris Manchester [:chmanchester] from comment #4)
> Henrik asked that I look into this. Both he and David are on holiday. I
> don't think renaming this is necessary, but if it's requested I certainly
> don't have a problem with it.
> 
> ato, if you are around this week perhaps you could lend an opinion on this
> matter? Thanks!

What's the reason we couldn't use getCurrentWindowHandle for both content- and chrome context?

Comment 6

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andreas Tolfsen (:ato) from comment #5)
> (In reply to Chris Manchester [:chmanchester] from comment #4)
> > Henrik asked that I look into this. Both he and David are on holiday. I
> > don't think renaming this is necessary, but if it's requested I certainly
> > don't have a problem with it.
> > 
> > ato, if you are around this week perhaps you could lend an opinion on this
> > matter? Thanks!
> 
> What's the reason we couldn't use getCurrentWindowHandle for both content-
> and chrome context?

This "chrome" windows are for OS level windows. We'd like to be able to track these as well as top level browsing contexts.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Personally I'm not in a position to justify this change. It's a simply RFE from my side to make the method names more consistent. So the final decision should be up to Andreas and David I assume.

Comment 8

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/1655/#review1423

Ship It!

Comment 9

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5da7240111cc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5da7240111cc

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1114793 - Rename marionette's chrome_window_handle to current_chrome_window_handle.