Issue 29365879: Issue 4616 - Push Edge related changes to the edge bookmark of adblockpluschrome (Closed)
|
Created:
Dec. 1, 2016, 9:35 a.m. by Oleksandr Modified:
Jan. 12, 2017, 10:07 a.m. CC:
Vasily Kuznetsov Visibility:
Public. |
DescriptionIssue 4616 - Push Edge related changes to the edge bookmark of adblockpluschrome
Patch Set 1 #Patch Set 2 : Add appropriate icons #
Total comments: 7
Patch Set 3 : Fix typo in README.md #
Total comments: 2
Patch Set 4 : Address comments #
Total comments: 1
Patch Set 5 : Make sure the package actually works correctly #
Total comments: 4
Patch Set 6 : Rebase and inherit from metadata.chrome #
Total comments: 3
Patch Set 7 : Refresh README.md #
Total comments: 6
Patch Set 8 : Adapt the README.md #
Total comments: 7
Patch Set 9 : More README.md extravaganza #Patch Set 10 : Hardcode the version number for Edge #
MessagesTotal messages: 21
https://codereview.adblockplus.org/29365879/diff/29366560/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode30 README.md:30: _adblockpluschrome-1.2.3.nnnn.crx_ or _adblockplussafari-1.2.3.nnnn.safariextz_ Nit: The second "or" should now be a comma I guess. https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode59 README.md:59: In Edge you should load _devenv.chrome as unpacked extension directory. After Nit: Missing the underscore after "devenv.chrome". https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode59 README.md:59: In Edge you should load _devenv.chrome as unpacked extension directory. After I think this paragraph should be above the Safari one. Also I think it should start with a sentence explaining that the Edge platform does not have it's own devenv command and that the Chrome one should be used instead. https://codereview.adblockplus.org/29365879/diff/29366565/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29366565/metadata.edge#newco... metadata.edge:31: logo_150.png = icons/detailed/abp-150.png I don't see icons/details/abp-150.png in the repository?
https://codereview.adblockplus.org/29365879/diff/29366560/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode30 README.md:30: _adblockpluschrome-1.2.3.nnnn.crx_ or _adblockplussafari-1.2.3.nnnn.safariextz_ On 2016/12/01 12:11:01, kzar wrote: > Nit: The second "or" should now be a comma I guess. Isn't it better with first "or" changed to comma? https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode59 README.md:59: In Edge you should load _devenv.chrome as unpacked extension directory. After On 2016/12/01 12:11:01, kzar wrote: > Nit: Missing the underscore after "devenv.chrome". Done. https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode59 README.md:59: In Edge you should load _devenv.chrome as unpacked extension directory. After On 2016/12/01 12:11:01, kzar wrote: > I think this paragraph should be above the Safari one. Also I think it should > start with a sentence explaining that the Edge platform does not have it's own > devenv command and that the Chrome one should be used instead. Done. https://codereview.adblockplus.org/29365879/diff/29366565/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29366565/metadata.edge#newco... metadata.edge:31: logo_150.png = icons/detailed/abp-150.png On 2016/12/01 12:11:01, kzar wrote: > I don't see icons/details/abp-150.png in the repository? Done.
Otherwise LGTM. Sebastian you want to take a look as well? https://codereview.adblockplus.org/29365879/diff/29366560/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29366560/README.md#newcode30 README.md:30: _adblockpluschrome-1.2.3.nnnn.crx_ or _adblockplussafari-1.2.3.nnnn.safariextz_ On 2016/12/01 14:18:04, Oleksandr wrote: > On 2016/12/01 12:11:01, kzar wrote: > > Nit: The second "or" should now be a comma I guess. > > Isn't it better with first "or" changed to comma? Yes, whoops that's what I meant. https://codereview.adblockplus.org/29365879/diff/29366698/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29366698/README.md#newcode54 README.md:54: _devenv.chrome_ as unpacked extension directory. After rebuilding the extension Nit: Missing a word "as AN unpacked extension directory".
Patch set 5: I have noticed that while installing the resulting package works, there are some issues with how it works. Mainly the icons were missing. This makes sure that everything actually works as expected after the install.
https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge#newco... metadata.edge:29: icons/abp-16.png = chrome/icons/abp-16.png I'm guessing you copied most of this from metadata.chrome? If so perhaps it would make more sense to inherit that file instead of metadata.common here?
https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge#newco... metadata.edge:29: icons/abp-16.png = chrome/icons/abp-16.png On 2016/12/08 12:37:31, kzar wrote: > I'm guessing you copied most of this from metadata.chrome? If so perhaps it > would make more sense to inherit that file instead of metadata.common here? Main reason I did not inherit from metadata.chrome is because we don't need the devtools stuff in Edge.
https://codereview.adblockplus.org/29365879/diff/29367022/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29367022/README.md#newcode53 README.md:53: For Edge you can use the same development environment as Chrome. You should load This is no longer accurate I suppose once #4720 is fixed. https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29367022/metadata.edge#newco... metadata.edge:29: icons/abp-16.png = chrome/icons/abp-16.png On 2016/12/09 06:43:00, Oleksandr wrote: > On 2016/12/08 12:37:31, kzar wrote: > > I'm guessing you copied most of this from metadata.chrome? If so perhaps it > > would make more sense to inherit that file instead of metadata.common here? > > Main reason I did not inherit from metadata.chrome is because we don't need the > devtools stuff in Edge. Fair enough, I guess duplicating it doesn't matter too much for now. (When we're ready to merge Edge into master we might reconsider, perhaps some of these should live in metadata.common or another common metadata file.)
After a rebase it turned out that since we have dropped jshydra
`require("devtools")` now throws if a module is not available. So I am just
inheriting from metadata.chrome in the end, since all modules have to be
available whether we use them or not.
https://codereview.adblockplus.org/29365879/diff/29367291/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29367291/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera Please read through the README and correct it. Since you rebased a few parts are wrong now.
https://codereview.adblockplus.org/29365879/diff/29367291/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29367291/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera On 2016/12/13 09:28:59, kzar wrote: > Please read through the README and correct it. Since you rebased a few parts are > wrong now. I do not see and any rebase-related artifacts you mention, unfortunately. However I did find a few inconsistencies and corrected those.
https://codereview.adblockplus.org/29365879/diff/29367291/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29367291/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera On 2016/12/14 02:08:45, Oleksandr wrote: > On 2016/12/13 09:28:59, kzar wrote: > > Please read through the README and correct it. Since you rebased a few parts > are > > wrong now. > > I do not see and any rebase-related artifacts you mention, unfortunately. > However I did find a few inconsistencies and corrected those. OK I'll add inline comments. https://codereview.adblockplus.org/29365879/diff/29367485/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera Please change this title to add Edge. https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode22 README.md:22: Run the following command in the project directory: There are more than one commands now, but this still uses the singular "command". (See Patch Set 1, before the rebase, for how it should be.) https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode35 README.md:35: development environment. For that run the following command: Same here, there are now multiple commands. https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode40 README.md:40: This will create a _devenv.chrome_ directory in the repository. In Chrome you This is wrong now, since it will create either a _devenv.chrome_ directory or a _devenv.edge_ directory depending on the command. Again see the Patch set 1 before the rebase. https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode45 README.md:45: For Edge you should load _devenv.edge/Extension_ as an unpacked extension directory. After Nit: This line is too long, please wrap at 80 characters. https://codereview.adblockplus.org/29365879/diff/29367485/README.md#newcode46 README.md:46: rebuilding the extension you should usually force reloading it in Edge. Please be a bit more specific here about when + how you should force reload. (If you should always force a reload then just say that.) Maybe mention why it's required too?
Patch Set 8 : Adapt the README.md You're right, I did miss a bunch of changes in the README.md. Fixed that now. Thanks.
https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge#newcode4 metadata.edge:4: [general] As long as Adblock Plus is still in beta, and still not released out of upstream, the version should remain 0.9.*. So we have to override the "version" key in the "general" section.
https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge#newcode4 metadata.edge:4: [general] On 2016/12/19 11:48:34, Sebastian Noack wrote: > As long as Adblock Plus is still in beta, and still not released out of > upstream, the version should remain 0.9.*. So we have to override the "version" > key in the "general" section. Our current version is 0.9.9.9. What would you see the next version to be? Besides, the version built from this *will* be based on the upstream, so I think it's a good time to switch to the correct version number now.
https://codereview.adblockplus.org/29365879/diff/29368711/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29368711/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera and Edge Nit: Second "and" should be a comma. https://codereview.adblockplus.org/29365879/diff/29368711/README.md#newcode29 README.md:29: _adblockpluschrome-1.2.3.nnnn.crx_ or adblockplusedge-1.2.3.nnnn.appx. Please surround the example edge filename with underscores too.
https://codereview.adblockplus.org/29365879/diff/29368711/README.md File README.md (right): https://codereview.adblockplus.org/29365879/diff/29368711/README.md#newcode1 README.md:1: Adblock Plus for Chrome and Opera and Edge On 2016/12/19 13:34:17, kzar wrote: > Nit: Second "and" should be a comma. Done. https://codereview.adblockplus.org/29365879/diff/29368711/README.md#newcode29 README.md:29: _adblockpluschrome-1.2.3.nnnn.crx_ or adblockplusedge-1.2.3.nnnn.appx. On 2016/12/19 13:34:17, kzar wrote: > Please surround the example edge filename with underscores too. Done.
Once Sebastian's comment regarding the version number is addressed this LGTM.
https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29365879/diff/29368711/metadata.edge#newcode4 metadata.edge:4: [general] On 2016/12/19 12:41:33, Oleksandr wrote: > On 2016/12/19 11:48:34, Sebastian Noack wrote: > > As long as Adblock Plus is still in beta, and still not released out of > > upstream, the version should remain 0.9.*. So we have to override the > "version" > > key in the "general" section. > Our current version is 0.9.9.9. What would you see the next version to be? > Besides, the version built from this *will* be based on the upstream, so I think > it's a good time to switch to the correct version number now. Our current version is 0.9.9.0 (not 0.9.9.9), and the next version would be 0.9.10. As far as version numbers matter in the first place, the 0.* notation is _correct_ and important to indicate that we are in beta, which isn't going to change before we merged the branch upstream and before we get the storage logic straight which then again requires the Windows Creators Update. Moreover, we might introduce conflicts when the upstream branch (i.e. tip) and the edge branch are on the same version track but are released separately.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
