Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
102273006:
Refactor Chromoting.java to pull out some nested classes. (Closed)
|
Created:
7 years ago by Lambros Modified:
6 years, 11 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor Chromoting.java to pull out some nested classes.
Flattening some of these nested classes will make it simpler to fix the
referenced bug. There are also some minor style cleanups.
This also adds a private lock, since the previous implementation was
synchronizing on the Chromoting object itself, and the newly-refactored
code would otherwise trigger a FindBugs warning.
The nested class HostListDirectoryGrabber will be addressed in a
followup CL.
BUG=304719
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242931
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move HostListAdapter to separate file #
Total comments: 8
Patch Set 3 : Remove spaces around = #
Total comments: 2
Patch Set 4 : Add TODO #
Messages
Total messages: 16 (0 generated)
I discovered that we were accessing mHosts without synchronization (and possibly others, I haven't checked). I can fix that here, or in another CL, whichever you prefer.
lgtm https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:369: class HostListAdapter extends ArrayAdapter<JSONObject> { Does this compile? I think you need to put it to a separate file.
not lgtm. clicked it by mistake.
https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:369: class HostListAdapter extends ArrayAdapter<JSONObject> { On 2013/12/21 02:15:28, Sergey Ulanov wrote: > Does this compile? I think you need to put it to a separate file. It's legal according to: http://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.6 You can have multiple classes defined in a .java file, with either public or unspecified (package) access. But at most one of them can be public. The compiler may enforce the restriction that the extra non-public classes can only be used from the same source file. So the non-public class is effectively a private implementation detail of the public class. If we think the extra class is truly a Pimpl, it makes sense to keep it in the same file (otherwise we should move it to another file as you suggested). And then, if we keep it in the same file, it can either be a separate class, or a static nested class. I prefer a separate class in this case, as it's better encapsulated than a nested class, which could access private members of its outer class. http://stackoverflow.com/questions/2148686/non-public-top-level-class-vs-stat...
https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:369: class HostListAdapter extends ArrayAdapter<JSONObject> { This isn't really a Pimple (pimple = Private IMPLEmentation). Anyway, even if it's allowed by the language, it still makes this code less readable. I think it would be better to move it to a separate file. Google Java style explicitly prohibits putting more than one top-level class in a single file (http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3-source-fi...) : A source file consists of, in order: - License or copyright information, if present - Package statement - Import statements - Exactly one top-level class On 2013/12/26 19:29:46, Lambros wrote: > On 2013/12/21 02:15:28, Sergey Ulanov wrote: > > Does this compile? I think you need to put it to a separate file. > It's legal according to: > http://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.6 > > You can have multiple classes defined in a .java file, with either public or > unspecified (package) access. But at most one of them can be public. > > The compiler may enforce the restriction that the extra non-public classes can > only be used from the same source file. So the non-public class is effectively a > private implementation detail of the public class. > > If we think the extra class is truly a Pimpl, it makes sense to keep it in the > same file (otherwise we should move it to another file as you suggested). > > And then, if we keep it in the same file, it can either be a separate class, or > a static nested class. I prefer a separate class in this case, as it's better > encapsulated than a nested class, which could access private members of its > outer class. > http://stackoverflow.com/questions/2148686/non-public-top-level-class-vs-stat...
https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/1/remoting/android/java/src/or... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:369: class HostListAdapter extends ArrayAdapter<JSONObject> { On 2013/12/26 20:03:48, Sergey Ulanov wrote: > This isn't really a Pimple (pimple = Private IMPLEmentation). Anyway, even if > it's allowed by the language, it still makes this code less readable. I think it > would be better to move it to a separate file. Google Java style explicitly > prohibits putting more than one top-level class in a single file > (http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3-source-fi...) > : > A source file consists of, in order: > - License or copyright information, if present > - Package statement > - Import statements > - Exactly one top-level class > > On 2013/12/26 19:29:46, Lambros wrote: > > On 2013/12/21 02:15:28, Sergey Ulanov wrote: > > > Does this compile? I think you need to put it to a separate file. > > It's legal according to: > > http://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.6 > > > > You can have multiple classes defined in a .java file, with either public or > > unspecified (package) access. But at most one of them can be public. > > > > The compiler may enforce the restriction that the extra non-public classes can > > only be used from the same source file. So the non-public class is effectively > a > > private implementation detail of the public class. > > > > If we think the extra class is truly a Pimpl, it makes sense to keep it in the > > same file (otherwise we should move it to another file as you suggested). > > > > And then, if we keep it in the same file, it can either be a separate class, > or > > a static nested class. I prefer a separate class in this case, as it's better > > encapsulated than a nested class, which could access private members of its > > outer class. > > > http://stackoverflow.com/questions/2148686/non-public-top-level-class-vs-stat... > OK, done!
https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:55: private Object mLock = new Object(); Do we really need this lock? It won't be necessary if we access mAccount and mToken only on the UI/main thread. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:108: new HostListDirectoryGrabber(this), mNetwork); The last parameter here specifies the thread on which the callback will be called. If you pass null it will be called on the main thread. Then you wouldn't need the lock. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:212: private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> { While you are removing nested classes, you can get rid of this one too. I think it can be replaced with a single method and an inline implementation of AccountManagerCallback<>. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: target.setText(Html.fromHtml(host.getString("hostName") + " (<font color = \"" + nit: remove spaces around = Ideally we should also cleanup how hosts are shown. Currently we show status string received from the server, and it's not supposed to be used this way (e.g. it's not localized). Instead of showing state as a string it might be better to have a green/red icon. That's not related to this CL.
https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:55: private Object mLock = new Object(); On 2013/12/28 02:17:30, Sergey Ulanov wrote: > Do we really need this lock? It won't be necessary if we access mAccount and > mToken only on the UI/main thread. We still need the network thread. I don't know if that thread needs to access these references, or whether we could pass copies of these data. I guess a lock is probably simpler for now. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:108: new HostListDirectoryGrabber(this), mNetwork); On 2013/12/28 02:17:30, Sergey Ulanov wrote: > The last parameter here specifies the thread on which the callback will be > called. If you pass null it will be called on the main thread. Then you wouldn't > need the lock. We still need the lock, regardless of what we pass to AccountManager. Our callback performs a network operation to grab the host-list, and that needs to run on a non-UI thread. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:212: private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> { On 2013/12/28 02:17:30, Sergey Ulanov wrote: > While you are removing nested classes, you can get rid of this one too. I think > it can be replaced with a single method and an inline implementation of > AccountManagerCallback<>. The problem is that the nested class has some internal state: boolean mAlreadyTried, and its run() method invokes the AccountManager a second time, passing "this" as a callback parameter (with mAlreadyTried set to true). The "single method" that you propose, wouldn't work as a method of the outer class, as its "this" reference would be the wrong thing. https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/102273006/diff/90001/remoting/android/java/sr... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: target.setText(Html.fromHtml(host.getString("hostName") + " (<font color = \"" + On 2013/12/28 02:17:30, Sergey Ulanov wrote: > nit: remove spaces around = Done. > > Ideally we should also cleanup how hosts are shown. Currently we show status > string received from the server, and it's not supposed to be used this way (e.g. > it's not localized). Instead of showing state as a string it might be better to > have a green/red icon. That's not related to this CL. Filed https://code.google.com/p/chromium/issues/detail?id=331103 for this.
lgtm, but see my comment. https://codereview.chromium.org/102273006/diff/160001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/160001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:212: private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> { This class performs two separate functions 1. saving token we get from the authenticator 2. fetching host list. (1) can happen on UI thread. (2) needs to be on the mNetwork thread because it's using blocking APIs for HTTP request. Ideally we should refactor the host-list logic into class that would provide async interface usable on UI thread. That way you wouldn't need to worry about threading in the UI code and the lock wouldn't be required here. It doesn't need to happen in this CL, but please add a TODO.
https://codereview.chromium.org/102273006/diff/160001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/102273006/diff/160001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:212: private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> { On 2014/01/03 00:53:05, Sergey Ulanov wrote: > This class performs two separate functions > 1. saving token we get from the authenticator > 2. fetching host list. > > (1) can happen on UI thread. (2) needs to be on the mNetwork thread because it's > using blocking APIs for HTTP request. Ideally we should refactor the host-list > logic into class that would provide async interface usable on UI thread. That > way you wouldn't need to worry about threading in the UI code and the lock > wouldn't be required here. It doesn't need to happen in this CL, but please add > a TODO. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/102273006/...
Retried try job too often on linux_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/102273006/...
Retried try job too often on linux_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/102273006/...
Message was sent while issue was closed.
Change committed as 242931 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
