Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(94)

Issue 6180766664884224: Issue 1663 - Made first-run page use an asynchronous messaging protocol (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by Wladimir Palant
Modified:
4 years, 11 months ago
Visibility:
Public.

Description

I considered your interface proposal here but I guess you will notice a few inconsistencies. Most important one is that we should keep the current messaging API, at least for now.

Patch Set 1 : #

Total comments: 42

Patch Set 2 : More common approach to wrapping code #

Patch Set 3 : Documentation fixes #

Patch Set 4 : Adressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+307 lines, -70 lines) Patch
A README.md View 1 2 3 1 chunk +56 lines, -0 lines 0 comments Download
A ext/content.js View 1 2 3 1 chunk +154 lines, -0 lines 0 comments Download
M firstRun.js View 1 2 3 3 chunks +97 lines, -70 lines 0 comments Download

Messages

Total messages: 12
Wladimir Palant
4 years, 11 months ago (2014-12-16 14:07:43 UTC) #1
Sebastian Noack
I suppose you stick with ext.*, just because the new UI abstraction layer isn't implemented ...
4 years, 11 months ago (2014-12-16 14:35:07 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js#newcode145 firstRun.js:145: if (!/[.\/]adblockplus\.org$/.test(event.origin)) On 2014/12/16 14:35:07, Sebastian Noack wrote: > ...
4 years, 11 months ago (2014-12-16 15:17:18 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js#newcode145 firstRun.js:145: if (!/[.\/]adblockplus\.org$/.test(event.origin)) On 2014/12/16 15:17:18, Wladimir Palant wrote: > ...
4 years, 11 months ago (2014-12-16 15:25:33 UTC) #4
Wladimir Palant
I changed the way ext/content.js wraps its code, addressing the comment from http://codereview.adblockplus.org/6427347985104896/ here as ...
4 years, 11 months ago (2014-12-17 13:13:37 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js#newcode97 firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in ...
4 years, 11 months ago (2014-12-17 13:45:22 UTC) #6
Wladimir Palant
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js#newcode97 firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in ...
4 years, 11 months ago (2014-12-17 14:37:04 UTC) #7
Sebastian Noack
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firstRun.js#newcode97 firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in ...
4 years, 11 months ago (2014-12-17 15:08:55 UTC) #8
Thomas Greiner
Just to reiterate: The requirements for the interface were that it 1) could allow grouping ...
4 years, 11 months ago (2014-12-18 10:17:48 UTC) #9
Wladimir Palant
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/README.md File README.md (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/README.md#newcode46 README.md:46: setup), but it can also happen in the user's ...
4 years, 11 months ago (2014-12-18 19:31:35 UTC) #10
Thomas Greiner
LGTM http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/content.js File ext/content.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/content.js#newcode20 ext/content.js:20: function getURLParameters(data) On 2014/12/18 19:31:35, Wladimir Palant wrote: ...
4 years, 11 months ago (2014-12-19 10:53:38 UTC) #11
Wladimir Palant
4 years, 11 months ago (2014-12-19 13:32:37 UTC) #12
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/...
File ext/content.js (right):

http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/...
ext/content.js:63: switch (message.type)
On 2014/12/19 10:53:38, Thomas Greiner wrote:
> Looks good. Not sure about whether an array for "app.get" at least might not
be
> the better choice since instead of `message.what == "issues"` it's just as
easy
> to do `message.what.indexOf("issues") > -1`.

The problem isn't checking what was requested - the problem is rather sending
the result back in a meaningful way, as this API allows requesting multiple
things at the same time. After considering the possibilities here, all of them
seem unnecessarily complicated - particularly given that using app.get to
request multiple things at the same time seems rather unusual.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5