Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29366526: Issue 4680 - Use runtime.openOptionsPage when available (Closed)

Created:
Dec. 1, 2016, 10:13 a.m. by kzar
Modified:
Dec. 9, 2016, 9:11 a.m.
Visibility:
Public.

Description

Issue 4680 - Use runtime.openOptionsPage when available

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -24 lines) Patch
M chrome/ext/background.js View 1 chunk +33 lines, -24 lines 2 comments Download

Messages

Total messages: 4
kzar
Patch Set 1 (Note this logic doesn't make much sense yet, but after #4579 we ...
Dec. 1, 2016, 10:17 a.m. (2016-12-01 10:17:01 UTC) #1
Wladimir Palant
LGTM but please file a separate issue for this change and describe properly why we ...
Dec. 1, 2016, 11:08 a.m. (2016-12-01 11:08:42 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29366526/diff/29366527/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29366526/diff/29366527/chrome/ext/background.js#newcode617 chrome/ext/background.js:617: ext.showOptions = chrome.runtime.openOptionsPage; I wonder how/whether this can work? ...
Dec. 8, 2016, 3:26 p.m. (2016-12-08 15:26:42 UTC) #3
kzar
Dec. 9, 2016, 9:11 a.m. (2016-12-09 09:11:02 UTC) #4
Message was sent while issue was closed.
https://codereview.adblockplus.org/29366526/diff/29366527/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29366526/diff/29366527/chrome/ext/backgrou...
chrome/ext/background.js:617: ext.showOptions = chrome.runtime.openOptionsPage;
On 2016/12/08 15:26:42, Sebastian Noack wrote:
> I wonder how/whether this can work? The old code (now used as fallback) calls
> the callback with a Page object, plus it waits until the page has finished
> loading. I suppose you only tested the scenario when opening the options page
> from the popup menu, in which case the callback isn't used.

Damn you're right. Sorry I'll open a review to fix that regression now.

Powered by Google App Engine
This is Rietveld