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

Issue 29337729: Issue 2374 - Implemented Tweaks section in options page (Closed)

Created:
Feb. 25, 2016, 5:50 p.m. by Thomas Greiner
Modified:
March 18, 2016, 11:09 a.m.
CC:
kzar
Visibility:
Public.

Description

Issue 2374 - Implemented Tweaks section in options page

Patch Set 1 #

Total comments: 21

Patch Set 2 : #

Total comments: 6

Patch Set 3 : Addressed Manvel's comments #

Patch Set 4 : Rebased to 0e4b41190cf5 #

Patch Set 5 : Added show_devtools_panel preference #

Total comments: 3

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -70 lines) Patch
M README.md View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M background.js View 1 2 3 4 5 chunks +81 lines, -25 lines 0 comments Download
M locale/en-US/options.json View 1 2 3 4 3 chunks +16 lines, -4 lines 0 comments Download
M messageResponder.js View 1 2 3 4 7 chunks +43 lines, -18 lines 0 comments Download
M options.html View 1 2 3 4 5 1 chunk +17 lines, -4 lines 0 comments Download
M options.js View 1 2 3 4 6 chunks +96 lines, -8 lines 0 comments Download
M skin/options.css View 1 2 3 4 5 5 chunks +12 lines, -10 lines 0 comments Download

Messages

Total messages: 12
Thomas Greiner
Feb. 25, 2016, 5:56 p.m. (2016-02-25 17:56:27 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json#newcode152 locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" This feature should ...
Feb. 25, 2016, 7:23 p.m. (2016-02-25 19:23:35 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json#newcode152 locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" On 2016/02/25 19:23:35, ...
Feb. 26, 2016, 1:01 p.m. (2016-02-26 13:01:58 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/options.json#newcode152 locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" On 2016/02/26 13:01:58, ...
Feb. 26, 2016, 3:20 p.m. (2016-02-26 15:20:14 UTC) #4
saroyanm
https://codereview.adblockplus.org/29337729/diff/29337730/README.md File README.md (right): https://codereview.adblockplus.org/29337729/diff/29337730/README.md#newcode91 README.md:91: * `safariContentBlocker`: sets Safari content blocker mock API Detail: ...
Feb. 29, 2016, 2:34 p.m. (2016-02-29 14:34:40 UTC) #5
Thomas Greiner
https://codereview.adblockplus.org/29337729/diff/29337730/README.md File README.md (right): https://codereview.adblockplus.org/29337729/diff/29337730/README.md#newcode91 README.md:91: * `safariContentBlocker`: sets Safari content blocker mock API On ...
Feb. 29, 2016, 5:32 p.m. (2016-02-29 17:32:43 UTC) #6
saroyanm
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcode222 options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/02/29 17:32:43, Thomas Greiner ...
March 11, 2016, 2:55 p.m. (2016-03-11 14:55:59 UTC) #7
Sebastian Noack
Aaron and Lisa agreed to not have tooltips for the terms that are currently indicated ...
March 14, 2016, 4:30 p.m. (2016-03-14 16:30:22 UTC) #8
Thomas Greiner
I uploaded three new patches. https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcode222 options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> ...
March 15, 2016, 3:39 p.m. (2016-03-15 15:39:26 UTC) #9
saroyanm
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcode222 options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/03/15 15:39:25, Thomas Greiner ...
March 16, 2016, 10:59 a.m. (2016-03-16 10:59:19 UTC) #10
Thomas Greiner
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcode222 options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/03/16 10:59:18, saroyanm wrote: ...
March 16, 2016, 11:14 a.m. (2016-03-16 11:14:36 UTC) #11
saroyanm
March 16, 2016, 12:27 p.m. (2016-03-16 12:27:35 UTC) #12
LGTM

https://codereview.adblockplus.org/29337729/diff/29338351/options.js
File options.js (right):

https://codereview.adblockplus.org/29337729/diff/29338351/options.js#newcode1117
options.js:1117: "safari_contentblocker", "show_devtools_panel",
On 2016/03/16 11:14:36, Thomas Greiner wrote:
> On 2016/03/16 10:59:18, saroyanm wrote:
> > detail: please use two spaces for indentation, same applies to the similar
new
> > line indentation below.
> 
> Is this a personal preference? Note that those are two indentation levels
("Use
> 2 spaces per indentation level.") to indicate that this line is a continuation
> of the previous one.

No I just wasn't considering the Indentation level, just ignore my comment.

Powered by Google App Engine
This is Rietveld