1332899 - Port Bug 1329926 and remove getURIForKeyword API from SeaMonkey.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Frank-Rainer Grahl (:frg)]

Bug 1329926 removed getURIForKeyword.

In SeaMonkey it is still used in suite/common/search/engineManager.js

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1332899-getURIForKeyword.patch

Not 100% happy with the patch. 

The new API is asynchronous and so won't return a result. Using .then didn't work either very well so I just removed the while loop for the dialog. Updates work fine but in case the user selects an existing keyword an error message dialog will be shown and the edit dialog dismissed where he/she before had the chance to change the value. But I think in this case the impact is neglectable.

The corresponding routine in browser is editKeyword in mozilla/browser/components/preferences/in-content/search.js.

Comment 2

[Philip Chee]

Comment on attachment 8829243 [details] [diff] [review]
1332899-getURIForKeyword.patch

The reason we have updateKeyword as well as editKeyword is because we can edit a keyword via two paths. One via the "Edit Keyword" button. And one by double clicking on the keyword column at the appropriate row. Neil insisted on this which was a PITA. By the way you missed to call to updateKeyword in setCellText.

[1] What we can do is to have only one edit path like Firefox. Remove the Edit Keyword button and the updateKeyword function. If we do this we might add some help text telling the user that they can double click on the keyword field to edit or add a keyword. Perhaps a tooltip. Or we could make the "Edit Keyword" button just call setCellText() with the right parameters.

> Not 100% happy with the patch. 

> The new API is asynchronous and so won't return a result. Using .then didn't
> work either very well so I just removed the while loop for the dialog.
> Updates work fine but in case the user selects an existing keyword an error
> message dialog will be shown and the edit dialog dismissed where he/she
> before had the chance to change the value. But I think in this case the
> impact is neglectable.

[2] Or you could just convert this SQL to JavaScript:
https://hg.mozilla.org/mozilla-central/rev/7feb3cffb4a2#l2.12
(I hear you're a whiz at SQL ;) )

> The corresponding routine in browser is editKeyword in
> mozilla/browser/components/preferences/in-content/search.js.

I'd try [1] first with the Edit button calling setCellText() directly.

Comment 3

[Philip Chee]

> I'd try [1] first with the Edit button calling setCellText() directly.
Well actually we can just follow Firefox as in:
https://hg.mozilla.org/mozilla-central/rev/75afd4419cd3#l4.11

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1332899-getURIForKeyword-V2.patch

I wouldn't have thought in a 100 years that the field in the table is editable. 

> +    let tree = document.getElementById("engineList");
> +    let column = tree.columns.getColumnFor(document.getElementById("engineKeyword"));
> +    document.getElementById("engineList").startEditing(index, column);

Is there an easier way to get the column?

Comment 5

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 1329926
User impact if declined: keyword can not be edited.
Testing completed (on m-c, etc.): c-c c-a
Risk to taking this patch (and alternatives if risky): broken already.
String changes made by this patch: none

Ratty seems to be currently unavailable. IanN can you take a look and review if ok.

Comment 6

[Ian Neal]

Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

>+++ b/suite/common/search/engineManager.js
>+  selectEditKeyword: function engineManager_selectEditKeyword() {
>+    let selectedEngine = gEngineView.selectedEngine;
>     if (!selectedEngine)
>       return;
Can we not use gEngineView.selectedIndex instead and test it is not -1?

>+    let index = gEngineView._engineStore._getIndexForEngine(selectedEngine);
>+    let tree = document.getElementById("engineList");
>+    let column = tree.columns.getColumnFor(document.getElementById("engineKeyword"));
>+    document.getElementById("engineList").startEditing(index, column);
Can this last one be:
tree.startEditing(index, column);

r/a=me with those addressed/answered

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1332899-getURIForKeyword-V3.patch

Patch with issues addressed. r+ from IanN carried forward.

Comment 8

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/8112dc270fcad461fa471ae2387a40a95806d3a3
https://hg.mozilla.org/releases/comm-aurora/rev/7abca4531ba32e48d9a94a3cbd0dee0f2a545256
https://hg.mozilla.org/releases/comm-beta/rev/7c268ed6732e420a3bcd5f38d3fa807600b1ad6d
https://hg.mozilla.org/releases/comm-release/rev/a6e7681a0b0433fa027afd36606ddfad82756ef0
https://hg.mozilla.org/releases/comm-esr52/rev/91edc5e459cd540184c29fbae38d234776fc08ab

Comment 9

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

Drat accidently pushed this to c-r and c-esr52 too without approval. But it will work. Please let me know if I should back it out or set approve. Sorry. 

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: none still works in 2.49
Testing completed (on m-c, etc.): c-b to c-c
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none

Comment 10

[Ian Neal]

Comment on attachment 8833788 [details] [diff] [review]
1332899-getURIForKeyword-V2.patch

a=me assuming the code the patch relies on is on m-r/m-esr

Comment 11

[Frank-Rainer Grahl (:frg)]

Thanks. The api removed for Gecko 53 was very old. Everything the patch relies on was already in place. Retest with 2.49 was successful.