1091187 - Move selectionchange code in nsGlobalWindow::UpdateCommands

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Neil Deakin]

Bug 987040 made an unnecessary api change here and added code that doesn't have anything to do with updating commands. This should be undone and the code moved elsewhere.

Comment 1

[Jamie Flint]

Hi, I'd like to take this on as my first bug assuming its still valid.

From what I can see the affected code is here: 
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#9754

Can you give an explanation of the intended function of this code and the intended function of the code to be moved?

Comment 2

[Neil Deakin]

Hi Jamie, thanks for taking this on. Some of the work for this has already been done in another bug. The remaining work is fairly simple:

1. Remove the two unused arguments from nsGlobalWindow::UpdateCommands.
2. Remove the check that looks for "selectionchange" within this method.

Comment 3

[Jamie Flint]

Attachment: v1 Remove checks from UpdateCommands

Comment 4

[Neil Deakin]

Comment on attachment 8674765 [details] [diff] [review]
v1 Remove checks from UpdateCommands

>diff --git a/dom/interfaces/base/nsIDOMWindow.idl b/dom/interfaces/base/nsIDOMWindow.idl
...
> 
>   // XXX Should this be in nsIDOMChromeWindow?
>-  void                      updateCommands(in DOMString action,
>-                                           [optional] in nsISelection sel,
>-                                           [optional] in short reason);
>+  void                      updateCommands(in DOMString action);

You'll need to change the uuid on the interface when the interface is changed. See https://developer.mozilla.org/en-US/docs/Generating_GUIDs

Otherwise this looks good.

Comment 5

[Jamie Flint]

Attachment: dom-updatecommands-v1.patch

Updated UUID in idl file

Comment 6

[Neil Deakin]

Comment on attachment 8675579 [details] [diff] [review]
dom-updatecommands-v1.patch

OK!

Comment 7

[Daniel Holbert [:dholbert]]

Was this patch ready to land? Looks like this got stalled after being r+'d.

Comment 8

[Neil Deakin]

Yes, it should be ok, but we likely will need an updated patch.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 10

[Logan Rosen [:Logan]]

Attachment: Bug 1091187 - remove unused arguments and check from UpdateCommands r?#dom-core

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28398da7d5d6
remove unused arguments and check from UpdateCommands r=dom-core,webidl,masayuki,smaug

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/28398da7d5d6