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

Issue 29345232: Issue 4084 - Do not use @@bidi_dir directly in adblockplusui (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by Oleksandr
Modified:
2 years, 5 months ago
Visibility:
Public.

Description

Issue 4084 - Do not use @@bidi_dir directly in adblockplusui

Patch Set 1 #

Total comments: 1

Patch Set 2 : Also adapt the mock implementation #

Total comments: 1

Patch Set 3 : Do not pretent to support getMessage for @@ui_locale and @@bidi_dir #

Patch Set 4 : Rebase #

Total comments: 3

Patch Set 5 : Fix rebase artifacts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -14 lines) Patch
M background.js View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M ext/common.js View 1 2 3 2 chunks +2 lines, -12 lines 0 comments Download
M messageResponder.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 9
Oleksandr
3 years, 9 months ago (2016-05-29 02:16:56 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29345232/diff/29345233/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29345232/diff/29345233/messageResponder.js#newcode184 messageResponder.js:184: bidiDir = Utils.readingDirection; You have to change the mock ...
3 years, 9 months ago (2016-05-29 14:05:08 UTC) #2
Oleksandr
I guess just mocking the readingDirection is enough.
3 years, 9 months ago (2016-05-30 14:00:25 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29345232/diff/29345366/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29345232/diff/29345366/messageResponder.js#newcode184 messageResponder.js:184: bidiDir = Utils.readingDirection; I guess we should move the ...
3 years, 9 months ago (2016-05-30 14:16:57 UTC) #4
Oleksandr
It is a bit clumsy without getMessage, but I guess you do have a point. ...
3 years, 9 months ago (2016-05-30 17:01:33 UTC) #5
Sebastian Noack
LGTM
2 years, 6 months ago (2017-08-28 19:26:56 UTC) #6
Thomas Greiner
Just a small but critical typo and an optional suggestion. https://codereview.adblockplus.org/29345232/diff/29529763/background.js File background.js (right): https://codereview.adblockplus.org/29345232/diff/29529763/background.js#newcode98 ...
2 years, 6 months ago (2017-08-29 08:51:17 UTC) #7
Oleksandr
https://codereview.adblockplus.org/29345232/diff/29529763/background.js File background.js (right): https://codereview.adblockplus.org/29345232/diff/29529763/background.js#newcode98 background.js:98: reuturn bidiDir = /^(ar|fa|he|ug|ur)(-|$)/.test(this.appLocale) ? On 2017/08/29 08:51:17, Thomas ...
2 years, 6 months ago (2017-08-29 11:26:30 UTC) #8
Thomas Greiner
2 years, 6 months ago (2017-08-29 11:43:07 UTC) #9
LGTM
Sign in to reply to this message.

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