Revert 225008 "Fix a threading bug in the brlapi basec braille c..."

Revert 225008 "Fix a threading bug in the brlapi basec braille c..."

> Fix a threading bug in the brlapi basec braille controller
> 
> This CL enables brlapi support by default in builds for chromeos (it's already enabled by default in chromeos itself, so that part is a noop). This revealed a threading bug in the implementation which is also fixed my moving a file_path_watcher from the IO thread to the FILE threadd.
> 
> BUG=178559
> 
> Review URL: https://chromiumcodereview.appspot.com/24325002

TBR=plundblad@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225011

[xhwang, 2013-09-24 16:55:32]

Committed patchset #1 manually as r225011.

[plundblad, 2013-09-25 01:09:46]

lgtm

xhwang@chromium.org writes:
> Reviewers: Peter Lundblad,
> 
> Description:
> Revert 225008 "Fix a threading bug in the brlapi basec braille c..."
> 
> > Fix a threading bug in the brlapi basec braille controller
> 
> > This CL enables brlapi support by default in builds for chromeos (it's  
> > already
> enabled by default in chromeos itself, so that part is a noop). This  
> revealed a
> threading bug in the implementation which is also fixed my moving a
> file_path_watcher from the IO thread to the FILE threadd.
> 
> > BUG=178559
> 
> > Review URL: https://chromiumcodereview.appspot.com/24325002
> 
> TBR=plundblad@chromium.org
> 
> Please review this at https://codereview.chromium.org/23537068/
> 
> SVN Base: svn://svn.chromium.org/chrome/
> 
> Affected files (+14, -41 lines):
>    M     trunk/src/build/common.gypi
>    M      
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h
>    M      
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc
>    M      
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc
> 
> 
> Index: trunk/src/build/common.gypi
> ===================================================================
> --- trunk/src/build/common.gypi	(revision 225010)
> +++ trunk/src/build/common.gypi	(working copy)
> @@ -1784,11 +1784,6 @@
>           'arm_float_abi%': '',
>           'arm_thumb%': 0,
>         }],
> -
> -      # Enable brlapi by default for chromeos.
> -      [ 'chromeos==1', {
> -        'use_brlapi%': 1,
> -      }],
>       ],
> 
> 
> Index:  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc
> ===================================================================
> ---  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc	

> (revision 225010)
> +++  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc	

> (working copy)
> @@ -134,7 +134,7 @@
>   }
> 
>   void BrailleControllerImpl::PokeSocketDirForTesting() {
> -  OnSocketDirChangedOnIOThread();
> +  OnSocketDirChanged(base::FilePath(), false /*error*/);
>   }
> 
>   void BrailleControllerImpl::StartConnecting() {
> @@ -146,44 +146,26 @@
>     if (!libbrlapi_loader_.loaded()) {
>       return;
>     }
> -  // One could argue that there is a race condition between starting to
> -  // watch the socket asynchonously and trying to connect.  While this is  
> true,
> -  // we are actually retrying to connect for a while, so this doesn't  
> matter
> -  // in practice.
> -  BrowserThread::PostTask(
> -      BrowserThread::FILE, FROM_HERE, base::Bind(
> -          &BrailleControllerImpl::StartWatchingSocketDirOnFileThread,
> -          base::Unretained(this)));
> -  ResetRetryConnectHorizon();
> -  TryToConnect();
> -}
> -
> -void BrailleControllerImpl::StartWatchingSocketDirOnFileThread() {
> -  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
>     base::FilePath brlapi_dir(BRLAPI_SOCKETPATH);
>     if (!file_path_watcher_.Watch(
>             brlapi_dir, false, base::Bind(
> -              &BrailleControllerImpl::OnSocketDirChangedOnFileThread,
> +              &BrailleControllerImpl::OnSocketDirChanged,
>                 base::Unretained(this)))) {
>       LOG(WARNING) << "Couldn't watch brlapi directory " <<  
> BRLAPI_SOCKETPATH;
> +    return;
>     }
> +  ResetRetryConnectHorizon();
> +  TryToConnect();
>   }
> 
> -void BrailleControllerImpl::OnSocketDirChangedOnFileThread(
> -    const base::FilePath& path, bool error) {
> -  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> +void BrailleControllerImpl::OnSocketDirChanged(const base::FilePath& path,
> +                                               bool error) {
> +  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> +  DCHECK(libbrlapi_loader_.loaded());
>     if (error) {
>       LOG(ERROR) << "Error watching brlapi directory: " << path.value();
>       return;
>     }
> -  BrowserThread::PostTask(
> -      BrowserThread::IO, FROM_HERE, base::Bind(
> -          &BrailleControllerImpl::OnSocketDirChangedOnIOThread,
> -          base::Unretained(this)));
> -}
> -
> -void BrailleControllerImpl::OnSocketDirChangedOnIOThread() {
> -  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
>     LOG(INFO) << "BrlAPI directory changed";
>     // Every directory change resets the max retry time to the appropriate  
> delay
>     // into the future.
> Index:  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h
> ===================================================================
> ---  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h	

> (revision 225010)
> +++  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h	

> (working copy)
> @@ -52,9 +52,7 @@
>     // Tries to connect and starts watching for new brlapi servers.
>     // No-op if already called.
>     void StartConnecting();
> -  void StartWatchingSocketDirOnFileThread();
> -  void OnSocketDirChangedOnFileThread(const base::FilePath& path, bool  
> error);
> -  void OnSocketDirChangedOnIOThread();
> +  void OnSocketDirChanged(const base::FilePath& path, bool error);
>     void TryToConnect();
>     void ResetRetryConnectHorizon();
>     void ScheduleTryToConnect();
> @@ -65,11 +63,12 @@
>     void DispatchKeyEvent(scoped_ptr<KeyEvent> event);
>     void DispatchOnDisplayStateChanged(scoped_ptr<DisplayState> new_state);
> 
> +  LibBrlapiLoader libbrlapi_loader_;
>     CreateBrlapiConnectionFunction create_brlapi_connection_function_;
> 
>     // Manipulated on the IO thread.
> -  LibBrlapiLoader libbrlapi_loader_;
>     scoped_ptr<BrlapiConnection> connection_;
> +  base::FilePathWatcher file_path_watcher_;
>     bool started_connecting_;
>     bool connect_scheduled_;
>     base::Time retry_connect_horizon_;
> @@ -77,9 +76,6 @@
>     // Manipulated on the UI thread.
>     ObserverList<BrailleObserver> observers_;
> 
> -  // Manipulated on the FILE thread.
> -  base::FilePathWatcher file_path_watcher_;
> -
>     friend struct DefaultSingletonTraits<BrailleControllerImpl>;
> 
>     DISALLOW_COPY_AND_ASSIGN(BrailleControllerImpl);
> Index:  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc
> ===================================================================
> ---  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc	

> (revision 225010)
> +++  
>
trunk/src/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc	

> (working copy)
> @@ -88,7 +88,7 @@
>       return true;
>     }
> 
> -  virtual int ReadKey(brlapi_keyCode_t* key_code) OVERRIDE {
> +  virtual int ReadKey(brlapi_keyCode_t* key_code) {
>       if (!data_->pending_keys.empty()) {
>         int queued_key_code = data_->pending_keys.front();
>         data_->pending_keys.pop_front();
> @@ -120,7 +120,7 @@
> 
>   class BrailleDisplayPrivateApiTest : public ExtensionApiTest {
>    public:
> -  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
> +  virtual void SetUpInProcessBrowserTestFixture() {
>       ExtensionApiTest::SetUpInProcessBrowserTestFixture();
>       connection_data_.connected = false;
>       connection_data_.display_size = 0;
> 
> 

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.